Re: [jira] Updated: (RIVER-203) ClassDep should always recognize '/' as a valid file separator character, regardless of OS

[Mark Brouwer <ma...@marbro.org>]

Hi Holger,

I don't have the time right now to have a good look at the patch itself
(sorry for that), but I would like to give some quick comments on your work.

Holger Hoffstätte (JIRA) wrote:
>      [ https://issues.apache.org/jira/browse/RIVER-203?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
> 
> Holger Hoffstätte updated RIVER-203:
> ------------------------------------
> 
>     Attachment: ClassDep-normalizedFileSeparators.patch
> 
> First cut to recognize '/' in class directory arguments. Basically
 > just replaces all references to the always-native File.separator(Char)
 > to a unified '/'. This should work on all platforms since apparently
 > Unix-style paths are always recognized correctly in Java, regardless
 > of native conventions.

I don't have a good feeling about this as the above is not in line with
the semantics for java.io.File:

"When a pathname string is converted into an abstract pathname, the
names within it may be separated by the default name-separator character
or by any other name-separator character that is supported by the
underlying system."

The default name separator for Windows is '\' but '/' is supported by
the file system, but there might be operating systems for which this is
not the case. Although I don't know of one, I still think fixing this
issue should be targeted for Windows only or for any other OS we know of
that accepts other name-separator characters than the default one.

> The output of the -files option is still printed in platform-native
 > form; see main() for the relevant comment. While this could also
 > easily be changed to the generic form, it would probably confuse
 > native consumers of the output (scripts?) so I left it as is.

I agree this is the proper behavior.

> Since I could not find any tests I tested this manually in eclipse
 > with several command lines. On Windows, passing e.g. "-cp
C:/path/to/jdk/lib/tools.jar;./classes -files
./classes/com/sun/jini/constants -in com.sun" as launcher arguments
 > now works and prints exactly the four classes in that package; adding
 > more -in arguments like e.g. "-in net." will still print the relevant
 > dependencies from the net.* package namespace (two in this case), etc.

I think that if portable scripts (in the context of Windows and Unix) is 
what one wants, that one should be able to use ':' as path separator. So 
the classpath below should work as well, if not I can't see what is gained.

   C:/path/to/jdk/lib/tools.jar:./classes

> Please let me know if this still produces the expected results in
 > "real" situations.
> 
> Note that I have not yet adapted any comments or javadocs except for
 > the two or three that I added. Also the -newdirbehaviour option does
 > not seem to work at all (or I misunderstood it, more likely).

The -newdirbehaviour option works for me, could you explain why you
think it is not working at all? As I added this option it can be the
case that the documentation is not clear enough.

> 
>> ClassDep should always recognize '/' as a valid file separator character, regardless of OS
>> ------------------------------------------------------------------------------------------
>>
>>                 Key: RIVER-203
>>                 URL: https://issues.apache.org/jira/browse/RIVER-203
>>             Project: River
>>          Issue Type: Improvement
>>          Components: com_sun_jini_tool
>>            Reporter: Jools Enticknap
>>            Priority: Minor
>>         Attachments: ClassDep-normalizedFileSeparators.patch
>>
>>
>> When specifying directories in Windows, ClassDep requires that such directories contain '\' as the file separator character.  
>> Java, on the other hand, also recognizes '/' as a file separator character in Windows.  
>> Changing ClassDep to also recognize '/' as a file separator character in Windows would lessen the number of changes required to port scripts between OSes.
-- 
Mark

Re: [jira] Updated: (RIVER-203) ClassDep should always recognize '/' as a valid file separator character, regardless of OS

[Holger Hoffstätte <ho...@wizards.de>]

Hi -

I was wondering why nobody answered me when I realized that I hadn't sent 
my own reply yet. grmbl :(

Mark Brouwer wrote:
>  > First cut to recognize '/' in class directory arguments. Basically
>  > just replaces all references to the always-native File.separator(Char)
>  > to a unified '/'. This should work on all platforms since apparently
>  > Unix-style paths are always recognized correctly in Java, regardless
>  > of native conventions.
> 
> I don't have a good feeling about this as the above is not in line with
> the semantics for java.io.File:

Yes, I had the same uneasy feeling but the File.separator is used for both 
path separation and as recursion indicator. Because I didn't want to 
change more than necessary (the source is IMHO quite difficult to read due 
to its rather, err..eclectic style).
Anyway, I can try to make the mapping conditional to Windows.

> > Since I could not find any tests I tested this manually in eclipse
> > with several command lines. On Windows, passing e.g. "-cp
> > C:/path/to/jdk/lib/tools.jar;./classes -files
> > ./classes/com/sun/jini/constants -in com.sun" as launcher arguments
> > now works and prints exactly the four classes in that package; adding
> > more -in arguments like e.g. "-in net." will still print the relevant
> > dependencies from the net.* package namespace (two in this case), etc.
> 
> I think that if portable scripts (in the context of Windows and Unix) is 
> what one wants, that one should be able to use ':' as path separator. So 
> the classpath below should work as well, if not I can't see what is gained.
> 
>   C:/path/to/jdk/lib/tools.jar:./classes

The -cp flag is a VM flag and does not need to be fixed. Maybe you 
misunderstood the example command line - the actual argument to ClassDep 
is the directory (./classes/com/sun/..), NOT the -cp flag; that was only 
necessary to add the tools.jar to the classpath for testing. I could also 
have added it to the JDK and removed the flag. Also, for portable scripts 
one would usually use a build tool like ant or maven anyway, where the 
problem does not exist.
As much as I'd like to fix the VM, I don't think this is in scope of this 
bug ;)

>  > the two or three that I added. Also the -newdirbehaviour option does
>  > not seem to work at all (or I misunderstood it, more likely).
> 
> The -newdirbehaviour option works for me, could you explain why you
> think it is not working at all? As I added this option it can be the
> case that the documentation is not clear enough.

I'll try again but somehow it always complained about the usage.

Holger