You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2024/04/15 15:46:38 UTC
[PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
cstamas opened a new pull request, #469:
URL: https://github.com/apache/maven-resolver/pull/469
This seems more logical step, simply use File.setLastModified as it does not throws, and is used all over the 1.9.x codebase.
---
https://issues.apache.org/jira/browse/MRESOLVER-536
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "Jurrie (via GitHub)" <gi...@apache.org>.
Jurrie commented on PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#issuecomment-2057405527
To be 100% sure I have built this branch and tested it in my cluster. Works! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #469:
URL: https://github.com/apache/maven-resolver/pull/469
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#discussion_r1566025920
##########
maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporter.java:
##########
@@ -691,8 +690,7 @@ public void handle(CloseableHttpResponse response) throws IOException, TransferC
if (lastModifiedHeader != null) {
Date lastModified = DateUtils.parseDate(lastModifiedHeader.getValue());
if (lastModified != null) {
- Files.setLastModifiedTime(
- task.getDataFile().toPath(), FileTime.fromMillis(lastModified.getTime()));
+ task.getDataFile().setLastModified(lastModified.getTime());
Review Comment:
I disagree, as this is the _only_ place where `Files` is used, while in _many other places_ (see master PR, installer, deployer, etc) `File.setLastModifiedTime` is used instead. IMHO this PR just "aligns" this one cuckoo egg with the rest. The change from `File` to `Path` (and `Files`) happened as part of 2.x work...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#discussion_r1566964022
##########
maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporter.java:
##########
@@ -691,8 +690,7 @@ public void handle(CloseableHttpResponse response) throws IOException, TransferC
if (lastModifiedHeader != null) {
Date lastModified = DateUtils.parseDate(lastModifiedHeader.getValue());
if (lastModified != null) {
- Files.setLastModifiedTime(
- task.getDataFile().toPath(), FileTime.fromMillis(lastModified.getTime()));
+ task.getDataFile().setLastModified(lastModified.getTime());
Review Comment:
While using path or file is something I don't care debating I think tracing there was an error and therefore the lastmodified value is wrong is valuable in the log so please log it (`if (!setlastmodified) log.trace()`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#discussion_r1566010743
##########
maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporter.java:
##########
@@ -691,8 +690,7 @@ public void handle(CloseableHttpResponse response) throws IOException, TransferC
if (lastModifiedHeader != null) {
Date lastModified = DateUtils.parseDate(lastModifiedHeader.getValue());
if (lastModified != null) {
- Files.setLastModifiedTime(
- task.getDataFile().toPath(), FileTime.fromMillis(lastModified.getTime()));
+ task.getDataFile().setLastModified(lastModified.getTime());
Review Comment:
This will hide the actual exception. I'd move the exception to trace level
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#discussion_r1566975286
##########
maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporter.java:
##########
@@ -691,8 +690,7 @@ public void handle(CloseableHttpResponse response) throws IOException, TransferC
if (lastModifiedHeader != null) {
Date lastModified = DateUtils.parseDate(lastModifiedHeader.getValue());
if (lastModified != null) {
- Files.setLastModifiedTime(
- task.getDataFile().toPath(), FileTime.fromMillis(lastModified.getTime()));
+ task.getDataFile().setLastModified(lastModified.getTime());
Review Comment:
@rmannibucau As I told to @michael-o , I find this proposal futile, and here is why: user @Jurrie spotted this problem ONLY HERE as this was the only one spot using method that throws (`Files.setLastModified`). If you look at master PR https://github.com/apache/maven-resolver/pull/468 it will show you all the places where time mtime is set (as master was ported from `File` to `Path`). So all those place will not log anything and are "forgiving" when they try but fail to set mtime, and hence, logging it only here makes no sense IMHO. If you want, you can make a PR for 1.9.x that sweep changes all `File.setLastModified` usages and logs something at TRACE, but I find it useless change (as it never logged that so far).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#discussion_r1566025920
##########
maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporter.java:
##########
@@ -691,8 +690,7 @@ public void handle(CloseableHttpResponse response) throws IOException, TransferC
if (lastModifiedHeader != null) {
Date lastModified = DateUtils.parseDate(lastModifiedHeader.getValue());
if (lastModified != null) {
- Files.setLastModifiedTime(
- task.getDataFile().toPath(), FileTime.fromMillis(lastModified.getTime()));
+ task.getDataFile().setLastModified(lastModified.getTime());
Review Comment:
I disagree, as this is the _only_ place where `Files` is used, while in _many other places_ (see master PR, installer, deployer, etc) `File.setLastModifiedTime` is used instead. IMHO this PR just "aligns" this one cuckoo egg with the rest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1.9.x] [MRESOLVER-536] Do not belly up if FS does not support setting mtime [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #469:
URL: https://github.com/apache/maven-resolver/pull/469#discussion_r1566975705
##########
maven-resolver-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporter.java:
##########
@@ -691,8 +690,7 @@ public void handle(CloseableHttpResponse response) throws IOException, TransferC
if (lastModifiedHeader != null) {
Date lastModified = DateUtils.parseDate(lastModifiedHeader.getValue());
if (lastModified != null) {
- Files.setLastModifiedTime(
- task.getDataFile().toPath(), FileTime.fromMillis(lastModified.getTime()));
+ task.getDataFile().setLastModified(lastModified.getTime());
Review Comment:
For master I agree, will prep a followup PR that does log
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org