Quantcast

[GitHub] ant-ivy pull request #20: Fix IVY-1522

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #20: Fix IVY-1522

janmaterne
GitHub user jaikiran opened a pull request:

    https://github.com/apache/ant-ivy/pull/20

    Fix IVY-1522

    The commit here contains a potential fix for the issue reported in https://issues.apache.org/jira/browse/IVY-1522. This PR also contains a test case to verify the fix.
   
    **Note that this issue isn't reproducible on my \*nix system and as such the fix/PR itself needs to be tested on a Windows OS before being merged. That plus the fact that this commit deals with the part of the code which gets used in multiple places. Tests on my local \*nix have gone fine with this change and such don't seem to introduce any regressions.**
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jaikiran/ant-ivy ivy-1522

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant-ivy/pull/20.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #20
   
----
commit 47312947d3c2411aae8b26f44182d7256193558e
Author: Jaikiran Pai <[hidden email]>
Date:   2017-05-18T14:42:41Z

    IVY-1522 Fix FileUtil.normalize to only consider ":" if it's part of the filesystem root on Windows OS

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    BasicURLHandler crashed... ":" need to be URL encoded?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    Looking at the logs, the BasicURLHandler seems to have crashed because the Jenkins process was killed due to:
   
    ```
        [junit] Running org.apache.ivy.util.url.BasicURLHandlerTest
    **Build timed out (after 10 minutes). Marking the build as aborted.**
    [locks-and-latches] Releasing all the locks
    [locks-and-latches] All the locks released
    Build was aborted
    Recording test results
        [junit] Running org.apache.ivy.util.url.BasicURLHandlerTest
        [junit] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0 sec
        [junit] Test org.apache.ivy.util.url.BasicURLHandlerTest FAILED (crashed)
    ```
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user nlalevee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    I have increased the timeout to 20m


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    The latest build completed successfully https://builds.apache.org/job/Ivy-GithubPR/15/console.
   
    Coming to the question:
   
    > Why not sticking only to well-known versioning schemes: OSGi/Aether and semver?
   
    The commit here relates to a feature that used to work in previous releases and breaks in 2.4.0. I looked at the Ivy docs to see if there is a clear mention of what's allowed in revisions and what's not. So far I haven't found anything specific, but it's my understanding that this character isn't restricted from being used in the revision.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    I am afraid that Ivy allows for a misfeature here. Note that you must test this on Windows because colon is not allowed in NTFS filenames: https://kb.acronis.com/content/39790


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user nlalevee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    I agree with you both.
    Ivy has never specified a version scheme, and it is kind of its strength, it can be compatible with a lot of use case. We should continue to do so.
    But we shouldn't mess with the filesystem either, we should support at least the 3 main OS: Linux, Windows and OSX.
   
    So a correct fix for this issue might be harder. Like there is an URLEncoder, Ivy should probably have a FileNameEncoder which will be able to write any file names and read them back.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    I can assure you that colon in version has NEVER have worked on Windows. Test case
   
    ```
    import java.io.*;
   
    public class TestColonNTFS {
       public static void main(String[] args) throws IOException {
          try (Writer writer = new BufferedWriter(new OutputStreamWriter(new FileOutputStream("file:name.txt"), "utf-8"))) {
             writer.write("something");
          }
       }
    }
    ```
    produces
   
    ```
    >dir
    ...
    2017-05-20  20:39                 0 file
    ...
    >dir /r file
    ...
    2017-05-20  20:39                 0 file
                                      9 file:name.txt:$DATA
    ```
    And that is the correct behaviour of NTFS: create an [alternate data stream](https://msdn.microsoft.com/en-us/library/windows/desktop/aa364404). You can use that for versioning, for sure ;-) But this is not portable.
   
    Now, should we introduce a special treatment for reserved and unsafe URL characters, we will create a lock-in unless all other tools accept the same conventions, which is unlikely. And that defeats the very idea of flexibility.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    I think both me and Nicolas agree that this "fix" isn't going to solve the case where this and other similar characters can't be fully supported as long as we are using the module descriptor attributes (via patterns of course) for file names. That's not the goal of this PR.
   
    I haven't ever tested the issue at hand on Windows, so I can't say one way or the other, but  the issue noted in the JIRA was apparently working in a previous version of Ivy. This PR is an attempt to do 2 things:
   
    - Let the previously working user project, work again.
    - Change the implementation detail in that method, which is changed in this commit, to use a different way to identify file system root, instead of relying on the presence of a specific character to determine that. So irrespective of whether we support the colon (and other similar characters), this change probably still is worth it.
   
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    "Making something that worked previously on a specific platform in specific circumstances work again" is not a good argument, especially when the coverage of the test case is incomplete. Unless this worka on Linux, Windows and macOS, it deserves Jira status "Won't fix".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    On the second thought, if PR changes the way to identify the system root, then the test cases should be specifically for that, not for parsing of versions with reserved/unsafe URL characters.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user nlalevee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    So let's get out of scope of this PR the support of weird characters in version, organisation or module name. Especially since the set of supported characters depends on the filesystem, which is usually tied to an OS but is not a strictly good indicator.
   
    So @jaikiran, would you consider rewrite the unit test to just test FileUtil.dissect ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    >> So @jaikiran, would you consider rewrite the unit test to just test FileUtil.dissect ?
   
    Yes, certainly. I will update this PR with the testcase tomorrow.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    I've now updated this PR to not focus on the usage of `:` in versions and instead test the FileUtil's method itself.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
GitHub user jaikiran reopened a pull request:

    https://github.com/apache/ant-ivy/pull/20

    Fix IVY-1522

    The commit here contains a potential fix for the issue reported in https://issues.apache.org/jira/browse/IVY-1522. This PR also contains a test case to verify the fix.
   
    **Note that this issue isn't reproducible on my \*nix system and as such the fix/PR itself needs to be tested on a Windows OS before being merged. That plus the fact that this commit deals with the part of the code which gets used in multiple places. Tests on my local \*nix have gone fine with this change and as such don't seem to introduce any regressions.**
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jaikiran/ant-ivy ivy-1522

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant-ivy/pull/20.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #20
   
----
commit c1765c71ba4394597409de790bf4529ff97e866e
Author: Jaikiran Pai <[hidden email]>
Date:   2017-05-18T14:42:41Z

    IVY-1522 Fix FileUtil.normalize to not base its normalization logic on ":" character

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user jaikiran closed the pull request at:

    https://github.com/apache/ant-ivy/pull/20


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user asfgit closed the pull request at:

    https://github.com/apache/ant-ivy/pull/20


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #20: Fix IVY-1522

janmaterne
In reply to this post by janmaterne
Github user nlalevee commented on the issue:

    https://github.com/apache/ant-ivy/pull/20
 
    merged. Thank both of you.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...