FWD » Core Development » Conversion Tools

I used the same zfile_set.txt file for a conversion on Windows as Ubuntu, and the X directive was basically ignored. The below is a snippet:

D ./abl/ (*.[pPwW]|*.cls)

X ./abl/fwd-embedded-driver.p
X ./abl/possenet/*
...

This occurs with Hotel GUI, when you make sure to checkout possenet into the abl/ directory. All the possenet files are included in the conversion, even though the exclude is specified.

I tried some permutations of both zfile_set.txt and going back to a file-ignore-list.txt. I'll post results from the Windows system.

Debug it. The cause should be clear in the debugger.

It seems for each option (-X or -Z), you can try with Unix path separators or Windows separators, and with/without a wildcard character * at the end.

I know -X is deprecated, but perhaps including the circumstances may help.

With .\\abl\\possenet\\*, ConversionDriver -XD3 f2+m0+cb ./abl/ *.[pPwW] file-ignore-list.txt works correctly.

With .\\abl\\possenet\\, ConversionDriver -XD3 f2+m0+cb ./abl/ *.[pPwW] file-ignore-list.txt does not work correctly.

With ./abl/possenet/* and ./abl/possenet/, ConversionDriver -XD3 f2+m0+cb ./abl/ *.[pPwW] file-ignore-list.txt do not work correctly. In fact, there is Show the PatternSyntaxExceptionHide

Exception in thread "main" java.util.regex.PatternSyntaxException: Unknown character property name {o} near index 8
\.\abl\possenet\.*
        ^
        at java.base/java.util.regex.Pattern.error(Pattern.java:2028)
        at java.base/java.util.regex.Pattern.family(Pattern.java:2910)
        at java.base/java.util.regex.Pattern.sequence(Pattern.java:2156)
        at java.base/java.util.regex.Pattern.expr(Pattern.java:2069)
        at java.base/java.util.regex.Pattern.compile(Pattern.java:1783)
        at java.base/java.util.regex.Pattern.<init>(Pattern.java:1430)
        at java.base/java.util.regex.Pattern.compile(Pattern.java:1095)
        at com.goldencode.p2j.util.FileSpecList.<init>(FileSpecList.java:262)
        at com.goldencode.p2j.util.FileListFactory.createBlacklist(FileListFactory.java:396)
        at com.goldencode.p2j.convert.TransformDriver.processCommandLine(TransformDriver.java:1587)
        at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1299)

For the -Z option...

D ./abl/ (*.[pPwW]|*.cls|*.htm|*html)
X ./abl/possenet/*

D ./abl/ (*.[pPwW]|*.cls|*.htm|*html)
X ./abl/possenet/

D ./abl/ (*.[pPwW]|*.cls|*.htm|*html)
X .\\abl\\possenet\\

D ./abl/ (*.[pPwW]|*.cls|*.htm|*html)
X .\\abl\\possenet\\*

• The -X blacklist doesn't work with ./abl/possenet/ but does with ./abl/possenet/*.
• The -X blacklist doesn't work with .\\abl\\possenet\\ or .\\abl\\possenet\\*
• The -Z fileset works with X ./abl/possenet/* but doesn't with X ./abl/possenet/
• The -Z fileset doesn't work with X .\\abl\\possenet\\ or X .\\abl\\possenet\\*

From #5021-7:

All files in the include/exclude list must use Linux-style separator and this will be replaced with the running OS separator.

I expect that the specification for filelist would be the same, that Linux-style should work. I am not sure if the wildcard should be required or not. Regardless, nothing works under native Windows. In the Ubuntu running on Windows, the wildcard seems to be required.

Greg Shah wrote:

Debug it. The cause should be clear in the debugger.

I am working on that. I wanted to get a grip on the scenarios. Any helpful tips would be appreciated.

Any thoughts on the wildcard being required?

The wildcard being required is not something that is OK. Was that your question?

Greg Shah wrote:

The wildcard being required is not something that is OK. Was that your question?

Yes, but the fact it has been like this Linux for both -Z and legacy -X leads me to believe it was always intended to be required. Also, -x Ignore List wiki mentions:

Each line must reference either a single file name, or a set of zero or more files using the following wildcard characters:

I suggest that -Z behave the same.

No. Doing so would greatly reduce the possible combinations of input. There is no good functional reason to require it.

Also, -x Ignore List wiki mentions:

The ignore list is dead and it sucks because it is so limited. The new fileset approach has much more functionality, so you should not expect anything to match the ignore list concept.

In processFileSet the ExplicitFileList reference = new ExplicitFileList(caseSens) returns a filenames list retrieved from the OS which contains \ as the path separator. Meanwhile, the filter is still using the Linux-style separator /. I think that's where things are breaking down.

Don't use \\ anywhere, unless we really have to do so. Instead of shifting to the File.separatorChar, I would prefer if we shift to / everywhere (both in the ExplicitFileList and in the fileset processing. Then the comparisons should work better and the same input can be used on both Windows and Linux. Windows tolerates /, right? So it should work. And there will be no confusion with escape chars.

Then the only issue is possible drive letters.

Greg Shah wrote:

Don't use \\ anywhere, unless we really have to do so. Instead of shifting to the File.separatorChar, I would prefer if we shift to / everywhere (both in the ExplicitFileList and in the fileset processing. Then the comparisons should work better and the same input can be used on both Windows and Linux. Windows tolerates /, right? So it should work. And there will be no confusion with escape chars.

This should work, and I will investigate the current blacklist code, because it should be performing a very similar function.

Then the only issue is possible drive letters.

If Running the Front End Conversion is the best specification, it does indicate all filenames are relative to the project root, so a drive letter is a no-no. Do you see any other circumstances where a drive letter would be used?

Then the only issue is possible drive letters.

If Running the Front End Conversion is the best specification, it does indicate all filenames are relative to the project root, so a drive letter is a no-no.

Yes, that is a valid reference.

Do you see any other circumstances where a drive letter would be used?

No.

Hi Roger, I am trying to setup a Windows11 instance to start working on this issue and have been meeting with some issue. Is there any guide on how to setup a virtual machine and the testcases project properly? I got https://proj.goldencode.com/projects/p2j/wiki/Windows_11_Virtual_Machine_Setup_with_FWD_v4_and_the_Testcase_Project from Lorian, but most of the link point to outdated information or do not work at all (such as the bazaar link). I also got to step 10 and I can't find any msys.bat file or use sh.

Roger Borrello wrote:

Dănuț Filimon wrote:

Hi Roger, I am trying to setup a Windows11 instance to start working on this issue and have been meeting with some issue. Is there any guide on how to setup a virtual machine and the testcases project properly? I got https://proj.goldencode.com/projects/p2j/wiki/Windows_11_Virtual_Machine_Setup_with_FWD_v4_and_the_Testcase_Project from Lorian, but most of the link point to outdated information or do not work at all (such as the bazaar link). I also got to step 10 and I can't find any msys.bat file or use sh.

Bazaar is the biggest barrier. To a certain degree you can follow #8104-2 but I will also say I abandoned trying to get bzr working on Windows and instead opted to use WSL2 and dbash.sh to pull a Docker image for all bazaar needs. It is from ~/secure/code/gcd_systems project and attached here. Once you get that setup for any bzr needs, you can go back to the native Windows for testing.

From what I understood, Lorian doesn't use bazaar so it should not be a requirement. I just want to setup p2j and build it and then use the testcases project. I did setup libffi according to https://proj.goldencode.com/projects/p2j/wiki/Development_Environment_Setup#Install-libffi. And now I am copying the necesary branches to the vm to continue.

Roger, is there a minimal requirement for the w11 vm? I've made mine with 4gb base memory and 2 processors and it hangs after a while when running .\gradlew all.

Dănuț Filimon wrote:

Roger, is there a minimal requirement for the w11 vm? I've made mine with 4gb base memory and 2 processors and it hangs after a while when running .\gradlew all.

I'm running Windows 11 in VirtualBox with 4 CPUs and 8GB base memory and I did not find any issues, try increasing those slowly until it works.

Șerban Bursuc wrote:

Dănuț Filimon wrote:

Roger, is there a minimal requirement for the w11 vm? I've made mine with 4gb base memory and 2 processors and it hangs after a while when running .\gradlew all.

I'm running Windows 11 in VirtualBox with 4 CPUs and 8GB base memory and I did not find any issues, try increasing those slowly until it works.

Thanks, I increased mine to 8gb / 6 cpus and I am currently testing it again.

I managed to setup the w11 vm and reproduce the issue. Currently investigating.

I used the following zset:

D ./abl *.p

X ./abl/fwd-embedded-driver.p
X ./abl/test.p

• step 1: there are three files found

  [abl\fwd-embedded-driver.p, abl\start.p, abl\test.p]

• step 2 (exclude): ./ is not removed because it is .\ and after the following code:
  

           filter = StringHelper.convertToRegEx(filter);
           Pattern pattern = caseSens 
                                ? Pattern.compile(filter) 
                                : Pattern.compile(filter, Pattern.CASE_INSENSITIVE);
  

  filter = \./abl/fwd-embedded-driver\.p and the pattern is \./abl/fwd-embedded-driver\.p as displayed by the debugger.

I tried to do something similar to

path = path.replace('/', File.separatorChar);

filter = filter.replace('/', File.separatorChar);

Replacing the "/" with the file separator in step 2 will make the filter match the ".\" (./abl/fwd-embedded-driver.p becomes .\abl\fwd-embedded-driver.p) and the .\ will be correctly removed. Converting the filter to regex makes it abl\fwd-embedded-driver\.p, the pattern will be abl\fwd-embedded-driver\.p.

The pattern matches the filter and no targets are found in

         String[] targets = reference.listFilenames((target) ->
         {
            return pattern.matcher(target).matches();
         });

The patterns should not be file-system specific. In fact, we should only support the / as the path separator. The same filesets should work with any OS.

Greg Shah wrote:

The patterns should not be file-system specific. In fact, we should only support the / as the path separator. The same filesets should work with any OS.

I read #9863-12 and I am currently experimenting. First removing the path = path.replace('/', File.separatorChar); call and checking the outcome, then finding out where else it is used. I will post my results.

On Windows, even if the path is ./abl/, it will make it into .\abl when calling new File(path), disregarding the File.separator. File.separator can't be overriden, so working on turning / into \\ when working on Windows for processing seems like the only option.

We've been canonicalizing filenames to use / on Windows in the past and had not found this limitation.

Dănuț Filimon wrote:

Roger/Greg, please review.

I won't be able to really test until I get back to my Windows laptop, earliest Monday. So might as well press on with...

Let me know if I should work on supporting the X ./abl/folder1/ filter in this task, I will adjust the %Done if so.

See notes #9863-6 and following... so you should keep moving on that.

Let me know if I should work on supporting the X ./abl/folder1/ filter in this task, I will adjust the %Done if so.

Yes, this needs to be fixed.

Code Review Task Branch 9863a Revision 15979

The change filter = filter.replace('/', File.separatorChar); seems incorrect:

• When running on Linux:
  • If the filter has \ characters, they will not be replaced and the result will fail.
  • The replacement of / by / is a waste of CPU.
• When running on Windows:
  • The switch of / to \ will work but in our experience, the use of / already works on Windows so it is unnecessary.

I would rather convert all \ to / in both Windows and Linux and let the result just work naturally on both platforms.

If I replace the '\\' with '/', the only problem will be the pattern because the files in Windows will be using '\' and the pattern will be using '/'. If I keep the StringHelper change and only replace '/' with '\\' when the File.separatorChar is '\\', it should work. Another option is to replace '\\' with '/' in:

         String[] targets = reference.listFilenames((target) ->
         {
            return pattern.matcher(target).matches();
         });

As for using X ./abl/tmp/ I can do a check for the last character and add .* in the filter before creating the pattern.

I committed 9863a/15980. I reverted the changes from StringHelper and replaced '\\' with '/', also added a .* when the last character is "/". I'll have to test a few scenarios to confirm this fix works.

Greg, is the scenario

The -Z fileset doesn't work with X .\\abl\\possenet\\ or X .\\abl\\possenet\\*

Dănuț Filimon wrote:

Greg, is the scenario [...] something that should be supported? Using a single \ instead of \\ works properly, but the pattern created can't match when there are more.

For this, maybe I can replace any number of / (/+) with a single /. In this way, the pattern will be built correctly and match the files found.

If I replace the '\\' with '/', the only problem will be the pattern because the files in Windows will be using '\' and the pattern will be using '/'.

Windows can use /. We don't have to use \\ on Windows. Your tests show otherwise?

As for using X ./abl/tmp/ I can do a check for the last character and add .* in the filter before creating the pattern.

Didn't this already work on Linux? It sounds like you are adding a new feature.

Dănuț Filimon wrote:

Greg, is the scenario [...] something that should be supported? Using a single \ instead of \\ works properly, but the pattern created can't match when there are more.

If it is a valid filename (or pathname) in Windows or Linux, then we should honor it in this list of directives.

Normally, we use things like Utils.canonicalizePath() to eliminate the excess nonsense like some/path/./more/path or extra /// characters. Can't we just do that here before changing to /?

Greg Shah wrote:

If I replace the '\\' with '/', the only problem will be the pattern because the files in Windows will be using '\' and the pattern will be using '/'.

Windows can use /. We don't have to use \\ on Windows. Your tests show otherwise?

As for using X ./abl/tmp/ I can do a check for the last character and add .* in the filter before creating the pattern.

Didn't this already work on Linux? It sounds like you are adding a new feature.

X ./abl/tmp/ didn't work on Linux. #9863-34 mentioned this should be supported.

Greg Shah wrote:

Normally, we use things like Utils.canonicalizePath() to eliminate the excess nonsense like some/path/./more/path or extra /// characters. Can't we just do that here before changing to /?

This will be useful, I'll do that.

Greg Shah wrote:

If I replace the '\\' with '/', the only problem will be the pattern because the files in Windows will be using '\' and the pattern will be using '/'.

Windows can use /. We don't have to use \\ on Windows. Your tests show otherwise?

I missed this, the problem is that in the ExplicitFileList.filenames we'll have only filenames that use the "\" file separator. The ExplicitFileList doesn't use the "/" file separator when adding files to the collection. For example, in listImpl() it will create a File which will turn the "\" into "/" even if all filenames are set to use "/".

Additional note for #9863-43, Utils.canonicalizePath() doesn't work for .//abl//tmp//* but works for .//abl//tmp//. I can create a separate method in Utils for this.

Dănuț Filimon wrote:

Additional note for #9863-43, Utils.canonicalizePath() doesn't work for .//abl//tmp//* but works for .//abl//tmp//. I can create a separate method in Utils for this.

I've experimented and found filter = Paths.get(filter).normalize().toString(); which works well on Linux, but throws this error on Windows:

java.nio.file.InvalidPathException: Illegal char <*> at index 13: .//abl//tmp//*
    at sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
    at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
    at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
    at sun.nio.fs.WindowsPath.parse(WindowsPath.java:94)
    at sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:255)
    at java.nio.file.Paths.get(Paths.java:84)
    at com.goldencode.p2j.util.FileListFactory.processFileSet(FileListFactory.java:162)
    at com.goldencode.p2j.convert.TransformDriver.processCommandLine(TransformDriver.java:1604)
    at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1299)

Splitting the filter at * is not an option, the scenario where ? is used will throw the same error regardless. I also dropped the filter += ".*" that allowed the X ./abl/tmp/ to work since I am sure we do not want to add a feature which didn't exist before.

I am sure we do not want to add a feature which didn't exist before.

Is it a new feature? Or is it just making the feature work properly?

Greg Shah wrote:

I am sure we do not want to add a feature which didn't exist before.

Is it a new feature? Or is it just making the feature work properly?

In #9863-5, it is used for testing. It never worked, it can be considered a new feature.

Dănuț Filimon wrote:

Greg Shah wrote:

I am sure we do not want to add a feature which didn't exist before.

Is it a new feature? Or is it just making the feature work properly?

In #9863-5, it is used for testing. It never worked, it can be considered a new feature.

The fact that it didn't work isn't evidence that it was working as intended. This area is not well tested.

What did X ./abl/tmp/ do? What do you think it should do? I would expect it to exclude anything in that path from the conversion. Does it do that?

Greg Shah wrote:

Dănuț Filimon wrote:

Greg Shah wrote:

I am sure we do not want to add a feature which didn't exist before.

Is it a new feature? Or is it just making the feature work properly?

In #9863-5, it is used for testing. It never worked, it can be considered a new feature.

The fact that it didn't work isn't evidence that it was working as intended. This area is not well tested.

What did X ./abl/tmp/ do? What do you think it should do? I would expect it to exclude anything in that path from the conversion. Does it do that?

I'd expect it to use the same format as mentioned in this wiki https://proj.goldencode.com/projects/p2j/wiki/Running_the_Front_End_Conversion#-x-Ignore-List. It should allow * and ? to be used, but also mention specific files.

Currently X ./abl/tmp/ does nothing. It will not exclude any files from the specified folder since the pattern will not match (it would require the * in order for the pattern to be turned into .* and pick up any file).

Currently X ./abl/tmp/ does nothing. It will not exclude any files from the specified folder since the pattern will not match (it would require the * in order for the pattern to be turned into .* and pick up any file).

I think that is probably confusing. Customers might easily add it and expect it to do something. Roger did. And I looked at it and didn't see a problem.

That suggests we need to implement the implicit * wildcard there.

Greg Shah wrote:

Currently X ./abl/tmp/ does nothing. It will not exclude any files from the specified folder since the pattern will not match (it would require the * in order for the pattern to be turned into .* and pick up any file).

I think that is probably confusing. Customers might easily add it and expect it to do something. Roger did. And I looked at it and didn't see a problem.

That suggests we need to implement the implicit * wildcard there.

Got it, I can add back the filter += ".*" when the last character is "/". As for cases like .///abl///tmp///* or .///abl///tmp///ab??e.p, I will think of something to be able to normalize those type of paths.

• Paths should replace "\\" with "/" before any processing is done, at the same time, any path that uses /{2,} should replace it with a single /. This makes it so that the pattern is built correctly.
• cwd will always be ./ now.
• For the scenario where X ./abl/tmp/ is used, it should be clear that the user would like to exclude all files from that directory. This means that all filters that end with / will have a * appended which will be translated in .* to match all files in that directory.
• After the pattern is built, there's an issue with the pattern using /, but the filenames from the file list use the Windows separator ("\\"). Filtering should also replace "\\" with "/".

1. D ./abl/tmp/ : at row 1 in file zfile_set.txt is malformed. Expected
2. D ./abl/tmp : at row 1 in file zfile_set.txt is malformed.
3. X ./abl/tmp
4. D .\abl\tmp\ : at row 1 in file zfile_set.txt is malformed.
5. D .\abl\tmp : at row 1 in file zfile_set.txt is malformed.
6. X .\abl\tmp
7. X .\abl\tmp\start.p\ where start.p is a directory
8. X .\abl\tmp\start.p\ where start.p is a file

Dănuț Filimon wrote:

Some cases I tested/will test:

1. D ./abl/tmp/ : at row 1 in file zfile_set.txt is malformed. Expected
2. D ./abl/tmp : at row 1 in file zfile_set.txt is malformed.
3. X ./abl/tmp
4. D .\abl\tmp\ : at row 1 in file zfile_set.txt is malformed.
5. D .\abl\tmp : at row 1 in file zfile_set.txt is malformed.
6. X .\abl\tmp
7. X .\abl\tmp\start.p\ where start.p is a directory
8. X .\abl\tmp\start.p\ where start.p is a file

I'm also wondering if we should account for .. in the filters?

I'm also wondering if we should account for .. in the filters?

Canonicalization whould deal with things like . and ...

Greg Shah wrote:

I'm also wondering if we should account for .. in the filters?

Canonicalization whould deal with things like . and ...

It will fail if we have filters that use * or ?. I tried Utils.canonicalizePath() and filter = Paths.get(filter).normalize().toString();.

We still need to canonicalize the result. Please write a version of the processing that can handle it.

Code Review Task Branch 9863a Revisions 15980 through 15982

The changes are good.

1. I wonder if FLF.normalizeFilePath() should be in Utils. I know only this code uses it, but it seems generally useful. Its javadoc needs to explain the reason it is different from Utils.canonicalizePath() (to deal with the filter wildcards, right?).

2. This code in FLF:

         if (filterDir.isDirectory() && !filter.endsWith("/"))
         {
            filter += "/*";
         }

It handles the !filter.endsWith("/") case but don't we also need to handle the filter.endsWith("/") case?

Greg Shah wrote:

Code Review Task Branch 9863a Revisions 15980 through 15982

The changes are good.

1. I wonder if FLF.normalizeFilePath() should be in Utils. I know only this code uses it, but it seems generally useful. Its javadoc needs to explain the reason it is different from Utils.canonicalizePath() (to deal with the filter wildcards, right?).

I agree, I will move the method and update the javadoc.

2. This code in FLF:

[...]

It handles the !filter.endsWith("/") case but don't we also need to handle the filter.endsWith("/") case?

The

         if (filterDir.isDirectory() && !filter.endsWith("/"))
         {
            filter += "/*";
         }

I committed 9863a/15983. Moved normalizeFilePath() to Utils, removed the unnecessary endsWith() call.

I added a comment in 9863a/15984.

Regression testing completed, no issues found.

9863a can be merged to trunk.