FWD » Core Development » Conversion Tools

We will externalize the conversion list and use it to drive both the front end and the TRPL processing. It will be available via an API. That API will also include the ability to report status back and log various details. All of that will be associated with the specific file and can be viewed/rendered by reading the data from the API. This will facilitate a range of the features we need to build, including things like IDE support and dependency management. We won't do logging or any output to the console directly.

One feature that is needed is to be able to dynamically add or remove files from the conversion list. #7179 will need to remove files when failures occur. The dependency management in #7169 will need the ability to add and remove files.

Interface is p2j.convert.ConversionStatus. Applicable for packages (refs #5703#note-133):

src/com/goldencode/p2j/convert/
src/com/goldencode/p2j/e4gl/
src/com/goldencode/p2j/pattern/
src/com/goldencode/p2j/preproc/
src/com/goldencode/p2j/report/
src/com/goldencode/p2j/uast/
src/com/goldencode/p2j/schema/

Branch 7180a created from trun rev 14561.

TODO: add TransformDriver cmd line arg parsing to set the level of logging. Check #6921#note-167 for example where this will be useful.

Branch 7180a rebased from trunk 14584.

Branch 7180a rebased from trunk 14754.

Rebased 7180a to latest trunk/15737, the branch is now at revision 15739.

• The Artifact will represent one of the items from the conversion list (a file for example), while the SourceArtifact is a bit more complex since it will allow external modifications. While both are defined, they are not the same because the Artifact should be a non-modifiable structure.
• The ArtifactManager will provide a way for the SourceArtifact objects to be modified.

• Was my description of the added classes accurate?
• It looks like the intention for SourceArtifact is to be something similar to a wrapper for the base Artifact, am I right?
• ArtifactManager will be the where the API should be built (or where it should reach) to have access to all SourceArtifacts, am I right?
• Does the conversion list refer to the abl sources, ade and skeleton?
• What kind of data I should store for each artifact?
• We should be able to "dynamically add or remove files", should removing files remove dependent Artifacts that will be left over (e.g. convert test1.p which uses test2.p, test2.p is not used by other files so removing test1.p should also remove test2.p)?

Currently, I am note sure where to start, so my starting point will be based on the answers I receive. If I have more questions, I'll make sure to post.

• The Artifact will represent one of the items from the conversion list (a file for example), while the SourceArtifact is a bit more complex since it will allow external modifications. While both are defined, they are not the same because the Artifact should be a non-modifiable structure.

Artifact should probably be an interface to define all the common actions to inspect or modify (or delete) any kind of artifact.

We probably need a BaseArtifact parent class that implements Artifact and can leave some aspects as abstract.

SourceArtifact and others like it should subclass from BaseArtifact to implement the specific behavior for each type. I'd expect there may be other BaseArtifact subclasses like IntermediateArtifact or OutputArtifact.

• The ArtifactManager will provide a way for the SourceArtifact objects to be modified.

I would expect it to be more like a tool for listing artifacts, adding/removing them, lock/unlock to access an artifact exclusively (for edits). There will be list-oriented methods to help cleanup artifacts in a build or to get the list of a type of artifact so that the next stage of processing can work with that list.

As we define the dependency processing (#7169, #6082/#5630), this processing will evolve. In other words, our processing of dependencies will use this artifact infrastructure.

• ArtifactManager will be the where the API should be built (or where it should reach) to have access to all SourceArtifacts, am I right?

Yes, it will be used to make lists of them.

• Does the conversion list refer to the abl sources, ade and skeleton?

When I say the "conversion list", I mean the list generated for the ConversionDriver but really this is something that I want to be a more generic service for all of our tools that process a project. Today, the ConversionDriver@ uses our various modes like -z which interprets a list of directives that specify some element or elements that should be added or removed from the conversion list. We want to make sure this is general purpose. That means it can be used in code analytics and in conversion and in whatever...

I don't want us to have any specific knowledge about the 4GL in this artifacts package. The tools should be general. If we need more specific behavior we can always implement subclasses in the convert package or elsewhere.

• What kind of data I should store for each artifact?

For now I think it would start with the project relative name but we will soon evolve it to have more. For example, we may want the file-level ID (used by AST processing) to be known or managed at this level. Intermediate or output artifacts might have a link to a SourceArtifact that they are created from. Overlaps like this might suggest that IntermediateArtifact and OutputArtifact might need a common parent class.

• We should be able to "dynamically add or remove files", should removing files remove dependent Artifacts that will be left over (e.g. convert test1.p which uses test2.p, test2.p is not used by other files so removing test1.p should also remove test2.p)?

Sort of. I think we will rarely, if ever, want to delete true SourceArtifact like a test1.p or test2.p. But for artifacts that generated, we might want to delete all artifacts generated from one or more SourceArtifact instances.

Currently, I am note sure where to start, so my starting point will be based on the answers I receive. If I have more questions, I'll make sure to post.

Understood.

During the execution of the ConversionDriver, FileSpecList is used to build the conversion list in TransformDriver.processCommandLine(). But there are several scenarios where the FileSpecList depends on the TransformDriver.ListType. I will go with the default CMD_LINE for the moment, then expand depending on the scenario.

The idea is that the ConversionDriver will have to pickup the artifacts (the conversion list). At first it will only be the CMD_LINE type but I'd allow filtering the artifacts based on the original:

   /** Form in which to obtain the list of files. */
   protected static enum ListType { CMD_LINE, WHITELIST, BLACKLIST, FILESPEC, FILESET };

Created ArtifactHandler should extend org.eclipse.jetty.server.handler.AbstractHandler, thsi will be used to handle requests for handling the artifacts. I'd like to keep ArtifactManager for any internal work that needs to be done, that's why I added ArtifactHandler. (this is inspired from the tenant work, maintaining a pattern)

Is the idea here to provide a REST API for access to the ArtifactManager? It might be something we would want to do, but I had not planned it yet. The LSP will be a JSON-RPC protocol. Any REST API would also be something separate from the admin REST API which is used at runtime, whereas this API would be something related to conversion-time.

I'm just trying to understand the idea.

 protected static enum ListType { CMD_LINE, WHITELIST, BLACKLIST, FILESPEC, FILESET };

Please note that we intend to remove WHITELIST and BLACKLIST completely. If all our current conversion projects were moved away from them, I'd have use remove these modes immediately. The FILESET (as implemented in #5135) already provides a superset of the features needed for WHITELIST, BLACKLIST and even for FILESPEC. We will retain CMD_LINE and FILESPEC for command line convenience but all real project configurations would be expected to be handled by FILESET mode.

Greg Shah wrote:

Created ArtifactHandler should extend org.eclipse.jetty.server.handler.AbstractHandler, thsi will be used to handle requests for handling the artifacts. I'd like to keep ArtifactManager for any internal work that needs to be done, that's why I added ArtifactHandler. (this is inspired from the tenant work, maintaining a pattern)

Is the idea here to provide a REST API for access to the ArtifactManager? It might be something we would want to do, but I had not planned it yet. The LSP will be a JSON-RPC protocol.

Yes, the idea is to provide a REST API to the ArtifactManager since it will contain a lot of methods (delete, deleteById, deleteAll, modifyById, add, getById, getAll). My point was that I don't want to make all the functionalities available if I end up expanding, only the important ones.

Any REST API would also be something separate from the admin REST API which is used at runtime, whereas this API would be something related to conversion-time.

This makes sense.

I took note of #7180-19.

Committed 7180a/15740. Added artifact classes, added functionalities to ArtifactManager.

1. If I want to have the artifacts available before any processing it's done (e.g. conversion, code analytics) as mentioned in #7180-15, I'd like to start by modifying the conversion process to use the artifacts first. What are some other points of interest that should rely on the artifacts aside from the ConversionDriver? I'd like to dive into the code to understand how the artifacts will need to be used.
2. How should I go about building the artifacts? My idea is to create a target in the project build.xml to run the artifact generation. Do let me know if there's a better way.
3. The current implementation relies on a single lock to access the artifacts collection, but "lock/unlock to access an artifact exclusively (for edits)." still needs to be implemented. I need a lock for the collection since all of them may be required by another "step", and then independent locks for each artifact (ReentrantLock), is the collection being locked an expected scenario?
4. What is the difference between the SourceArtifact/IntermediateArtifact/OutputArtifact? I think the OutputArtifact is a read-only object, the source can be any external input as mentioned in the docs. What about the IntermediateArtifact. This will help me with adding a few more methods for creating these types of artifacts in ArtifactManager.

If I want to have the artifacts available before any processing it's done (e.g. conversion, code analytics) as mentioned in #7180-15, I'd like to start by modifying the conversion process to use the artifacts first. What are some other points of interest that should rely on the artifacts aside from the ConversionDriver? I'd like to dive into the code to understand how the artifacts will need to be used.

Any code that makes lists of inputs to process or somehow deals with these inputs. Code that uses FileList instances (ExplictFileList or FileSpecList). This would include TransformDriver/ConversionDriver/ProgressTransformDriver, ScanDriver, ReportDriver, SearchTrees, ASTGenerator, PatternEngine, CallGraphWorker, RootNodeList, ReportApi, ServiceSupport.

I agree with your idea that these pieces of code should be working with the new artifact approach and they should all get lists of the artifacts from the ArtifactManager.

How should I go about building the artifacts? My idea is to create a target in the project build.xml to run the artifact generation. Do let me know if there's a better way.

The build.xml is not something that is part of FWD. So we don't want logic or dependencies there. Instead, I expect that any of our "driver" programs will be modified to automatically call the ArtifactManager to build or edit or manipulate these lists as needed.

We will want to standardize the inputs to these drivers so that they all can be driven off the same modes. We should go ahead and plan to remove whitelist and blacklist so that we don't have to do work on things that have no value.

The current implementation relies on a single lock to access the artifacts collection, but "lock/unlock to access an artifact exclusively (for edits)." still needs to be implemented. I need a lock for the collection since all of them may be required by another "step", and then independent locks for each artifact (ReentrantLock), is the collection being locked an expected scenario?

Let's assume that a given collection is something which:

• Is built on the fly when a driver program is launched.
• It is built using inputs for the driver program and will define the initial set of artifacts to be processed. Currently we use the ExplictFileList or FileSpecList classes for this.
• It cannot be accessed by processing in another JVM. In other words, it is an in-memory collection.
• It can be accessed for modifications from more than one thread.
  • For example, when we multi-thread the conversion process, parsing an OO class on one thread will call processing in the DependencyManager to add artifacts to the collection for each OO class in the inheritance/implements tree (see #6082 and #7169).
  • This means it absolutely needs to be carefully synchronized, even for simple reads (getting an Artifact from the collection).
• Artifact level locking should be done directly on the Artifact instances themselves (as you note, it could be a ReentrantLock).

What is the difference between the SourceArtifact/IntermediateArtifact/OutputArtifact? I think the OutputArtifact is a read-only object, the source can be any external input as mentioned in the docs. What about the IntermediateArtifact. This will help me with adding a few more methods for creating these types of artifacts in ArtifactManager.

I expect that we should be able to make a list of any of these artifact types and they should be well known to our tools. Any kind of artifact that needs to be processed in some standard way should be represented as an artifact type than can be listed, processed, manipulated.

Source artifacts will be read-only. They will generally be read/parsed and turned into intermediate artifacts.

Intermediate artifacts might be read-only (once created) like the .cache file. But they also might be edited over thousands of steps like the .ast or .jast.

Output artifacts might be read-only like the antiparsed .java code BUT there might be things like the name_map.xml which are outputs that do get edited.

The code in com.goldencode.artifacts is not supposed to be specific to FWD. It is a generic set of functionality that can be used by drivers and tools in developer projects like FWD to implement some clean and consistent management of all the kind of artifacts that need to be managed.

I expect specialization in the users of com.goldencode.artifacts. I would expect the com.goldencode.p2j.convert package to create subclasses of SourceArtifact, IntermediateArtifact and OutputArtifact. For example, 4GL procedures, include files and classes all seem like types of SourceArtifact. Another kind of specialization might be using lambdas or interfaces to register some helper code that can override the behavior of the ArtifactManager. Or if it is at the Artifact level then the subclass design might handle it. We might also need to use the factory pattern here. Anyway, the point is that we want a clean way to extend the generic features of com.goldencode.artifacts for use in various packages in FWD.

Thanks for the input, I plan to start modifying the ConversionDriver first to make use of the artifacts.

Greg Shah wrote:

We will want to standardize the inputs to these drivers so that they all can be driven off the same modes. We should go ahead and plan to remove whitelist and blacklist so that we don't have to do work on things that have no value.

Is there an existent issue for this, or should I create one?

Looking at the conversion driver I can simply pass the FileList as a parameter to the ArtifactManager.createArtifacts() method and then take each file and create a BaseArtifact instance. I need to consider the scenario where there should not be an artifact generated for the same file, so I'll map the filename from the FileList to the Artifact id in another collection.

Another idea is to have a bulk create method that will lock the artifacts collection when adding several artifacts so that we do not lock each time we add a single artifact.

Is there an existent issue for this, or should I create one?

There isn't such a task. We can make the changes here or add a task.

Before we can make that move, we need to ensure that customer projects are all shifted away from using whitelists or blacklists. I perfer to have them migrate before we merge the branch to trunk which removes these modes. The idea is that we don't want a blocking revision in trunk that customers can't move to without breaking their projects. See #9725.

Looking at the conversion driver I can simply pass the FileList as a parameter to the ArtifactManager.createArtifacts() method and then take each file and create a BaseArtifact instance. I need to consider the scenario where there should not be an artifact generated for the same file, so I'll map the filename from the FileList to the Artifact id in another collection.

Another idea is to have a bulk create method that will lock the artifacts collection when adding several artifacts so that we do not lock each time we add a single artifact.

I think that the ArtifactManager (AM) should handle the creation and processing of the list, instead of the client code handling the FileList instances directly. The idea is that all the file list processing should probably be hidden away in the AM. The client code (like ConversionDriver or ReportDriver) should take standardized command line inputs and pass these to the AM to create and return the list of Artifacts to process.

By hiding that list creation process (which can require scanning the filesystem etc...), we can cleanly build the artifacts and manage locking as needed. The client code shouldn't really care anything about the individual filenames or the filesystem. It should just say "give me a list of the artifacts to process, based on these inputs". It should probably get back an iterator and then it can walk the individual elements as needed.

As we provide more specialized Artifact subclasses, we would expect to initialize the AM using some kind of factory that allows creation of those Artifact instances in a standard way.

As you'll see in the ConversionDriver, we initially process a list of .p, .w, .cls... 4GL program filenames and then create .cache files for these which we then parse. As we work through the parsing and shift into TRPL, we must process AST artifacts instead of 4GL input files. This kind of shift from one artifact type to another is something that the AM should handle (but which might need some specialization to handle the specifics).

Greg Shah wrote:

I think that the ArtifactManager (AM) should handle the creation and processing of the list, instead of the client code handling the FileList instances directly. The idea is that all the file list processing should probably be hidden away in the AM. The client code (like ConversionDriver or ReportDriver) should take standardized command line inputs and pass these to the AM to create and return the list of Artifacts to process.

I realized after reading the first note that the conversion should be based on the artifact usage and that the FileList should be integrated into the ArtifactManager.

By hiding that list creation process (which can require scanning the filesystem etc...), we can cleanly build the artifacts and manage locking as needed. The client code shouldn't really care anything about the individual filenames or the filesystem. It should just say "give me a list of the artifacts to process, based on these inputs". It should probably get back an iterator and then it can walk the individual elements as needed.

There are scenarios where we need to sort the artifacts based on the filename (for example AstGenerator.prioritize(list)) that should be centralized.

As we provide more specialized Artifact subclasses, we would expect to initialize the AM using some kind of factory that allows creation of those Artifact instances in a standard way.

As you'll see in the ConversionDriver, we initially process a list of .p, .w, .cls... 4GL program filenames and then create .cache files for these which we then parse. As we work through the parsing and shift into TRPL, we must process AST artifacts instead of 4GL input files. This kind of shift from one artifact type to another is something that the AM should handle (but which might need some specialization to handle the specifics).

Got it, the artifact factory will be a must when the implementation will require it (the need to differentiate and create different artifacts depending on the process - generating .cache files is a good example).

It seems my next commit will take a while, currently working my way into the conversion and it's a lot more that I expected. I reached this point in the conversion where TransformDriver.zapFiles() calls SymbolResolver.getClassFileList() and filters the artifacts. I have to make some easy, some hard decisions here and there, but I am slowly making progress. My plan is to make the 7180a compile and commit after a code cleanup and a successful hotel conversion.

Greg, I am afraid I am going too deep into the idea of "using artifacts instead of FileList" so let me ask a question which is now blocking me. I modified the TransformDriver.processCommandLine and moved the list processing to a method from ArtifactManager that will create a list of Artifact instances and is based on the file list option of the ConversionDriver, then we change the JobDefinition to use the artifacts list instead of the FileList like so:

-      return new JobDefinition(run, transform, files, cfgProfiles, threads, abortOnError, incremental, debug);
+      return new JobDefinition(run, transform, artifacts, cfgProfiles, threads, abortOnError, incremental, debug);

Let's ignore the huge amount of errors this will cause for the moment and let's focus on the question I have at hand. During the conversion, TransformDriver.cleanScanResults() is called and this code is called:

       zapFiles(source.listFilenames(), extslist);
       if (!job.incremental)
       {
         zapFiles(SymbolResolver.listFakeoutFiles(), extslist);
       }

Not sure how to address this scenario where I define new FileList in SymbolResolver.getClassFileList() and also make use of the DIR_CLASSES. Any advice?

PatternEngine also stores FileList instances...

private final ArrayList<FileList> astSpecs = new ArrayList<>();

Wherever we use a FileList instance, we will need to refactor it to use the AM. For example, the TransformDriver.cleanScanResults() will need to use call methods in the AM to do the actual "zapping". The users of the AM shouldn't directly manipulate the files. We will have to define tools in the AM to match our needs.

Greg Shah wrote:

Wherever we use a FileList instance, we will need to refactor it to use the AM. For example, the TransformDriver.cleanScanResults() will need to use call methods in the AM to do the actual "zapping". The users of the AM shouldn't directly manipulate the files. We will have to define tools in the AM to match our needs.

The alternative of the FileList will be a list of Artifact instances, this makes it difficult to refactor the PatternEngine because it will use

private final ArrayList<List<Artifact>> astSpecs = new ArrayList<>();

One possibility of integrating the FileList into the ArtifactManager is to create a method

public static List<Artifact> createArtifactsByFileList(Class<?> clazz, Object... args)

I've reached a stable version here I can build the 7180a branch, but there are a lot of conversion/import issues which I need to address.

The alternative of the FileList will be a list of Artifact instances, this makes it difficult to refactor the PatternEngine because it will use

A question: do we need to return a List<Artifact> here or can we return an Iterator<Artifact>? The reason I say this is because when we add in things like dependency processing, we don't want to directly edit the List. Instead we want to have some abstract process that modifies the current list.

But to do that we might need to return a class that is like a collection of Artifact instances but without true Java collection (direct access to the instances). Then we could use that instance as a reference to the collection, but operations on it are done indirectly by the AM.

Greg Shah wrote:

The alternative of the FileList will be a list of Artifact instances, this makes it difficult to refactor the PatternEngine because it will use

A question: do we need to return a List<Artifact> here or can we return an Iterator<Artifact>? The reason I say this is because when we add in things like dependency processing, we don't want to directly edit the List. Instead we want to have some abstract process that modifies the current list.

The Iterator<Artifact> can't be reset., this is the only problem I can see with this idea.

But to do that we might need to return a class that is like a collection of Artifact instances but without true Java collection (direct access to the instances). Then we could use that instance as a reference to the collection, but operations on it are done indirectly by the AM.

This will work, currently I only need to filter the artifacts so there is no add/remove operation. There's also a scenario where I have to take an artifact collection and create another collection from it (e.g. start.p -> start.p.ast). Nice idea! For the moment, I'll make an ArtifactCollection class that will store the list of artifacts.

• ConversionDriver.convertSourceNamesToJavaAstNames() requires three ArtifactCollection instances to be merged, the method for reference:
  

      private void convertSourceNamesToJavaAstNames()
      {
  -      List<String> oldjasts = new ArrayList<>();
  +      List<ArtifactCollection> oldjasts = new ArrayList<>();
  
         // merge the lists
  -      oldjasts.addAll(Arrays.asList(source.withSuffix(".jast")));
  -      oldjasts.addAll(Arrays.asList(frames));
  -      oldjasts.addAll(Arrays.asList(menus));
  -      
  +      oldjasts.add(ArtifactManager.convertArtifactsWithSuffix(source, ".jast"));
  +      oldjasts.add(ArtifactManager.getArtifactsByName(Arrays.asList(frames)));
  +      oldjasts.add(ArtifactManager.getArtifactsByName(Arrays.asList(menus)));
  +
         // save the results
  -      jasts = new ExplicitFileList(oldjasts.toArray(new String[0]));
  +      jasts = ArtifactManager.mergeArtifactCollections(oldjasts);
      }
  

  (similar scenario SymbolResolver.listFakuoutFiles(), GenerateLegacyProxyOpenClient.main())
• Had problems refactoring PatternEngine.gatherTargetPaths(). This one has to check the ArtifactCollection for existent files in the system using the relative name of the artifact so even if there are 60 artifacts in the collection, the targetPaths can end up with 20 elements. I removed part of the Warn if nothing to process and not in quiet mode. processing for the moment and have to add it back after refactoring it.
• ReportDriver.oldSyntax: had to comment the if (opt.ignore) because it uses FileListFactory.createBlacklist(), still working on refactoring this one but it's a lower priority since I am considering #9725.
• AstGenerator.processFile(), commented the following:
  

               if (list != null)
               {
  -               removeDuplicate(list, filename);
  -               
  -               // set the current list element to be the output filename
  -               // so that any downstream processing can find the AST
  -               list.set(idx, new File(filename));
  +//               removeDuplicate(list, filename);
  +//               
  +//               // set the current list element to be the output filename
  +//               // so that any downstream processing can find the AST
  +//               list.set(idx, new File(filename));
               }
  

  This also needs to be addressed.
• Allow ArtifactCollection to be sorted (similar to ExplicitFileList) - see ScanDriver.scan()
• SymbolResolver:
  

         /** Classes in the OO4GL_PATH, mapped qualified classname to filename. */
  -      private Map<String, String> oo4glCls = null;
  +      private Map<String, Artifact> oo4glCls = null;
  
         /** Classes in the DOTNET_PATH, mapped qualified classname to filename. */
  -      private Map<String, String> dotnetCls = null;
  +      private Map<String, Artifact> dotnetCls = null;
  
         /** Classes in the ASSEMBLY_PATH, mapped qualified classname to filename. */
  -      private Map<String, String> assemblyCls = null;
  +      private Map<String, Artifact> assemblyCls = null;
  

  Storing artifacts directly, this also needs to be fixed.
• many 110+ character lines
• left the documentation last

The only think that needs to be fixed is Generate Java Source Business Logic and Frames Classes step (the code does not compile as it is missing all items related to ui). I should be able to review and take care of the other mentioned problems after making sure the code converts and compiles (hotel project).

The Iterator<Artifact> can't be reset., this is the only problem I can see with this idea.

Do we ever need to reset it within the current phase of processing? If not, then we would just let each phase of processing get its own iterator. In between phases of processing, we can apply transformations either via an explicit processing step like where we calculate new names off the original source names OR we might just be editing as we go, for example by adding artifacts based on a dependency graph. Then when the next phase gets its iterator, it will naturally have everything needed to process and won't need to reset.

My objective here in using iterators is to ensure that the callers must go through the abtracted AM API instead of doing any of the artifact management themselves.

I've updated the code to use an ArticatCollection, some things I've remarked that might be of concern:

Understood. This is all expected. I think most of this gets pushed back into the artifacts implementation and we just need to provide the right/clean API to use it.

Allow ArtifactCollection to be sorted

FYI, when we have full dependency management, the sorting will only work within a given "component" of the dependency graph. Even within a specific "app", we will have things that can't process in sorted order. For example, OO classes must process top-down in the inheritance and implements trees. You can't process class A that extends class B and implements class C until both B and C are processed.

In regard to the FileListFactory.createBlacklist() (and the whitelist processing), perhaps we should remove that support in 7180a and just ensure that #9725 is complete before we merge.

Greg Shah wrote:

The Iterator<Artifact> can't be reset., this is the only problem I can see with this idea.

Do we ever need to reset it within the current phase of processing? If not, then we would just let each phase of processing get its own iterator. In between phases of processing, we can apply transformations either via an explicit processing step like where we calculate new names off the original source names OR we might just be editing as we go, for example by adding artifacts based on a dependency graph. Then when the next phase gets its iterator, it will naturally have everything needed to process and won't need to reset.

I wanted to say that we would need the artifacts to be sorted at some point, but based #7180-41 I think it will do. The only scenario where we need to reiterate the artifacts is when we parse the .p/.w files and once again when we create the respective .ast file in another processing step for the already parsed artifacts. ArtifactCollection is way cleaner and can implement Iterator<Artifact>.

What I did not manage to avoid was not adding elements to this collection in JavaPatternWorker$WorkArea.pfiles collection (now an ArtifactCollection) which is not ok, I am adding this problem to the #7180-38 list.

Had an issue with the import process on the hotel app, it was looking for a file in data/hotel.p2o in gatherTargetPaths(), but my implementation only creates artifacts for existent files in the system. The file could actually found in cvt/data/hotel.p2o. I changed the build_db.xml and this worked, but I will add to the list a way to create artifacts for files that do not exists in the system (I removed them because of the second bullet point mentioned in #7180-38).

We want to have the qualified filenames (even for newly created artifacts) to be managed (and even encapsulated) as much as possible by the AM and artifacts themselves. The idea here is that various tools that use the artifacts don't know or care where these things reside. That would mean that whenever we have processes that create new names, that process should be somehow inside the AM. I would expect the AM to create Artifact instances that have a transformed name and then the AM user could lock that Artifact instance and then get a stream to read or write to the actual file. The AM user doesn't need to know the actual name.

The idea here is to create a strong separation of concerns which will give us:

• consistency
• a clear implementation in common code
• the ability to change the storage implementation without changing any of the AM "client code"

Greg Shah wrote:

We want to have the qualified filenames (even for newly created artifacts) to be managed (and even encapsulated) as much as possible by the AM and artifacts themselves. The idea here is that various tools that use the artifacts don't know or care where these things reside. That would mean that whenever we have processes that create new names, that process should be somehow inside the AM. I would expect the AM to create Artifact instances that have a transformed name and

It's not the qualified filename that needs to be stored in the Artifact, the targetPaths scenario needs the absolute path...

then the AM user could lock that Artifact instance and then get a stream to read or write to the actual file. The AM user doesn't need to know the actual name.

This is what I will be working on after getting a feedback on my current work, I am now writing the javadoc and fixing a few scenarios which are a bit messy (this includes a bit of refactoring).

By "qualified", I mean a filename with a path. That path might be relative or absolute, we shouldn't care. Either way, the AM client code should not be aware of the filename.

Dănuț Filimon wrote:

My next steps:

• find more usages of FileList in drivers and replace them with artifacts
• find cases where we store possible artifacts in strings and make use of ArtifactCollection for them
• TODOs mentioned above (I want to investigate and take care of all of them if possible)

An additional important step derived from the TODO is avoiding duplicate artifacts for the same file and checking if the artifact is backed by a file that exists on the file system.

Greg Shah wrote:

Intermediate or output artifacts might have a link to a SourceArtifact that they are created from. Overlaps like this might suggest that IntermediateArtifact and OutputArtifact might need a common parent class.

I've got into this a bit when debugging in TransformDriver.executeJob(), when we have job.incremental we built dependencies for the files here, I'm thinking of already having those dependencies available for each artifact. I think this will simplify the building the dependencies process since it will only depend on one artifact collection (we can have a method in ArtifactManager to check each dependent artifact in the collection). I am not sure where we build these dependencies (looks like we might store them in the cvtdb?). Another idea is to take this if statement and move it into a method from ArtifactManager, but I'd like to rely on the artifacts more.

I tried to replace the SchemaConfig$NsConfig.importFile with an Artifact, eventually I ended up noticing that the SchemaConfig$NsConfig.name is based on the p2j.cfg.xml config file. The importFile can also be null, in which case we use the name, I am not sure I can refactor this and make use of artifacts at the moment, so I am noting this down for the future.

I managed to refactor a few points to avoid using removeAll() from ArtifactCollection. I need to fix another scenario in the import phase where the DataModelWorker.generateFilename() is called for the ./cvt/data/hotel in the conversion, while in the targetPaths because empty because the file is searched in data/hotel/ and not /cvt/data/hotel.

In the first case, the Artifact is created by TransformDriver.createDatabaseList() based on /home/ddf/gcd/hotel_gui/cvt/data/hotel.schema that comes from this line

String fileName = config.getSchemaFileName(schema, extension);

Greg, somehow I lost my commit from 7180a/15741 ...

ddf@ddfmachine:~/gcd/7180a$ bzr commit -m "Added more properties to Artifact, simplified artifact creation, refactored artifact dependency check, removed old methods already integrated in the ArtifactManager (refs: #7180)" 
brz: ERROR: Bound branch BzrBranch7(file:///home/ddf/gcd/7180a/) is out of date with master branch RemoteBranch(bzr+ssh://localhost:2224/opt/secure/code/p2j_repo/p2j/active/7180a/).                      
To commit to master branch, run update and then commit.
You can also pass --local to commit to continue working disconnected.
ddf@ddfmachine:~/gcd/7180a$ bzr bind
brz: ERROR: Branch is already bound
ddf@ddfmachine:~/gcd/7180a$ bzr status
modified:
  src/com/goldencode/artifacts/Artifact.java
  src/com/goldencode/artifacts/ArtifactCollection.java
  src/com/goldencode/artifacts/ArtifactManager.java
  src/com/goldencode/artifacts/BaseArtifact.java
  src/com/goldencode/p2j/convert/ConversionDriver.java
  src/com/goldencode/p2j/convert/TransformDriver.java
  src/com/goldencode/p2j/uast/JavaPatternWorker.java
  src/com/goldencode/p2j/uast/ScanDriver.java
ddf@ddfmachine:~/gcd/7180a$ bzr dif >7180a.patch
ddf@ddfmachine:~/gcd/7180a$ bzr update
All changes applied successfully.                                                                                                                                                                          
Updated to revision 15740 of branch bzr+ssh://localhost:2224/opt/secure/code/p2j_repo/p2j/active/7180a
Your local commits will now show as pending merges with 'brz status', and can be committed with 'brz commit'.
ddf@ddfmachine:~/gcd/7180a$ bzr status

I think I can recover this from the .~1~ files that are created on the revert.

I managed to recover my changes and commit them to 7180a/15471 + the ones I wanted to commit originally (I hope there will be no such problems in the future).

Committed 7180a/15742. I replaced all usages of FileList with ArtifactCollection, and only the ones remaining in the artifacts package are left.

Code Review Task Branch 7180a Revisions 15826 through 15828

The code is moving along nicely.

1. What is the purpose for Artifact.get|setExtName()? Is that supposed to be the fully qualified name? A name with a relative path? Something else? The method names and javadoc probably need enhancement.

2. Why Artifact.existsFile() instead of Artifact.exists()

3. ArtifactCollection.getArtifacts() (and AM.getArtifacts() should return a copy of the collection.

4. ArtifactCollection.iterator() should not iterate the collection directly.

5. ArtifactManager.createConversionArtifacts() should not call System.exit() directly. On failure, it should return a unique return code and let the caller decide how to process.

6. ArtifactManager.convertArtifactsWithSuffix() (both versions and also AM.createArtifactsForConfig() rely upon FileOperationsWorker.findFile(), the Configuration class and FileSystemDaemon.search(). All of these things are specific to FWD instead of generic filesystem processing. The processing should be abstracted. To the degree that the processing cannot be made generic, it should be customizable. For example, a lambda can be passed in which provides the search implementation.

7. AM.mustConvertArtifact() should not exist. We should add that as a feature of a subclass of Artifact which would be speciifc to FWD.

8. We should consider how we can remove the dependencies on Configuration in the AM. For now we may need to leave it, but we should think about it. Configuration is specific to FWD.

9. The AM.convertArtifactsWithSuffix() processing should be renamed and made more generic. The use of "convert" in the name is not inappropriate but it can confuse since it might be thought to refer to code that does conversion from 4GL to Java. Let's call this transformArtifactNames(). The other thing here is that the transformation should probably be delegated to the Artifact instance itself. That would allow a FWD-specific sublcass to implement specific logic instead of hard coding it in the AM.

10. The call to removeDuplicate() in AstGenerator.processFile() is needed.

11. The use of AstManager.lockTree() should be shifted to a lock() call on the Artifact.

12. The sorting in ScanDriver.scan() should be migrated to ArtifactManager.sorted().

As mentioned yesterday, I am trying to make way for direct usage of artifacts in the AstManager. The problem I am facing is calls to getFilename() used in places like ProgressParser.enum_stmt(), this uses the antlr Parser method and I am not sure how to approach this change. Greg, do you have any suggestions?

I wouldn't like to have wrapper methods to get the Artifact based on the filename, but I don't exclude this at the moment. The problem is that there are calls like this in *.g.

It also appears that TransformDriver.ExcludeCheck class and TransformDriver.ScanCheck class are never used.

As mentioned yesterday, I am trying to make way for direct usage of artifacts in the AstManager. The problem I am facing is calls to getFilename() used in places like ProgressParser.enum_stmt(), this uses the antlr Parser method and I am not sure how to approach this change. Greg, do you have any suggestions?

Consider this example code for instantiating the lexer and parser:

         SymbolResolver  sym    = new SymbolResolver(true);
         FileReader      fr     = new FileReader(fname);
         BufferedReader  br     = new BufferedReader(fr);
         ProgressLexer   lexer  = new ProgressLexer(br, sym);
         ProgressParser  parser = new ProgressParser(lexer, sym);

         // do something with the parser...

Instead of manually creating a stream, we should use a SourceArtifact (or possibly just an Artifact) and the artifact itself should be able to give us the stream (as well as answer the filename request). The lexer could take the Artifact as a parameter instead of the stream and it could handle getting the stream internally. The parser can then use the artifact in the lexer to get the filename when needed. We could override the ProgressParser.getFilename() to do this in one place.

I wouldn't like to have wrapper methods to get the Artifact based on the filename, but I don't exclude this at the moment. The problem is that there are calls like this in *.g.

Correct, we don't want to do that.

Dănuț Filimon wrote:

It also appears that TransformDriver.ExcludeCheck class and TransformDriver.ScanCheck class are never used.

They can be removed.

1. A Reader parameter given to the ProgressLexer constructor, this one calls this(new CharBuffer(kludge ? new DotKludgeReader(in) : in));
2. An InputStream parameter given to the ProgressLexer constructor, this one calls this(new ByteBuffer(kludge ? new DotKludgeStream(in) : in));

The filename is retrieved through the getFilename() call and this one relies on a ParserSharedInputState/LexerSharedInputState which holds the actual filename, so we can't effectively get the filename from the Artifact itself. For the case where the lexer received a StringReader... (e.g. attachSchemaValidation() - progress.g) it receives only text and there is no file involved.

We can however extent the lexer to take a SourceArtifact, make use of an Reader/InputBuffer parameter instead and make sure those generated constructors are properly used:

public ProgressLexer(InputStream in) {
    this(new ByteBuffer(in));
}
public ProgressLexer(Reader in) {
    this(new CharBuffer(in));
}
public ProgressLexer(InputBuffer ib) {
    this(new LexerSharedInputState(ib));
}
public ProgressLexer(LexerSharedInputState state) {
    super(state);
    caseSensitiveLiterals = false;
    setCaseSensitive(false);
    literals = new Hashtable();
}

1. ClearStream.java (2)
2. Preprocessor.java (1)
3. AstGenerator.java (2)

If we only consider the scenarios where the BufferedReader is built on an actual existent file, we can make this change. I expect the StringReader issue to come back in the future and cause a few problems. I've wrapped up a few changes and right now I want to just be able to build the FWD project so that I can commit and continue. I am also worried that when I override the getFilename() eventually I will have to check if I have an artifact or just a Reader (StringReader).

I also did not find any usages for the ProgressLexer(InputStream in ... ) in the project.

The filename is retrieved through the getFilename() call and this one relies on a ParserSharedInputState/LexerSharedInputState which holds the actual filename, so we can't effectively get the filename from the Artifact itself.

Did you override the getFilename() method to provide our own implementation?

For the case where the lexer received a StringReader... (e.g. attachSchemaValidation() - progress.g) it receives only text and there is no file involved.

This is a valid use case. Perhaps we provide a TransitoryArtifact which provides an in-memory artifact that meets the interface requirements without adding any filesystem nonsense.

I also did not find any usages for the ProgressLexer(InputStream in ... ) in the project.

Correct. We can't use InputStream because we need to treat these files as text and read them with the proper charset. Thus we direct all stream usage through the Reader approach.

Greg Shah wrote:

The filename is retrieved through the getFilename() call and this one relies on a ParserSharedInputState/LexerSharedInputState which holds the actual filename, so we can't effectively get the filename from the Artifact itself.

Did you override the getFilename() method to provide our own implementation?

Yes, I saved the SourceArtifact in the lexer and parser (I need them in both) and if the artifact is null, it would use super. Currently works well, but the idea was to pass the artifact instead of the filename so I also added a getArtifact() method.

For the case where the lexer received a StringReader... (e.g. attachSchemaValidation() - progress.g) it receives only text and there is no file involved.

This is a valid use case. Perhaps we provide a TransitoryArtifact which provides an in-memory artifact that meets the interface requirements without adding any filesystem nonsense.

We could also use IntermediateArtifact which already exists. I like this idea, just need to change the SourceArtifact usage to just an Artifact (should've been like that from the start).

I also did not find any usages for the ProgressLexer(InputStream in ... ) in the project.

Correct. We can't use InputStream because we need to treat these files as text and read them with the proper charset. Thus we direct all stream usage through the Reader approach.

Got it.

Committed 7180a/15829. Addressed part of the #7180-60 review, added locking methods for the artifact and the plan is now to try and replace filename usages and stop relying on methods like getByName() from ArtifactCollection and ArtifactManager.

I tested hotel conversion and switched to converting *.cls, I found a few regressions and now I am working on fixing them. I am currently investigating a 7180a regression in SymbolResolver.createClassMappings().

Committed 7180a/15830. Replaced a few more usages of the filename with artifacts, this expands the lexer/parser artifact usage.

Rebased 7180a to latest trunk/15846, the branch is now at revision 15853.

1. src/com/goldencode/p2j/uast/ScanDriver.java
   - Removed this class and integrated its functionality into ArtifactManager
2. src/com/goldencode/artifacts/ArtifactConversionResult.java
   - Created to avoid calling System.exit() directly when processing the conversion artifacts, the processing is successful when code = 1
3. src/com/goldencode/artifacts/Artifact.java
   - Removed getName(), setName(), @getExtName(), setExtName().
   - Renamed existsFile() to exists().
   - Added lock(), unlock() and tryLock().
4. src/com/goldencode/artifacts/ArtifactCollection.java
   - Modified iterator() and getArtifacts() to make use of a copy of the artifacts collection.
   - Added remove() and sort() methods.
5. src/com/goldencode/artifacts/ArtifactManager.java
   - Integrated ScanDriver.
   - Renamed a few methods.
   - The process of creating artifacts for conversion will make use of ArtifactConversionResult to return a code and the ArtifactCollection resulted.
   - Added addLexerArtifact() methods TODO: I am not fond of these methods, so I will work on replacing them. This is mostly because I ended up in a situation where I create a duplicate SourceArtifact when I already have a BaseArtifact.
   - Modified getArtifacts() to make use of a copy of the artifacts collection.
   - Added getArtifactByName(). TODO: Already working on getting rid of it, I needed it for the lexer/parser changes. I will be able to remove it when I manage to replace more usages of filenames with artifacts.
   - Removed sorted(), mustConverArtifact().
   - Added scan().
6. src/com/goldencode/artifacts/BaseArtifact.java
   - Removed name, extName, getName(), setName(), @getExtName(), setExtName().
   - Renamed existsFile() to exists().
   - Added lock(), unlock() and tryLock(), mustConvert(). TODO: move mustConvert() to Artifact.
7. src/com/goldencode/artifacts/SourceArtifact.java
   - Added getReader() and setReader(). TODO: The idea of using a TransitoryArtifact instead sounds better now, I will look into making this change.
8. src/com/goldencode/ast/AstManager.java
   - Moved the static initializer from ScanDriver here.
9. src/com/goldencode/p2j/convert/ConversionDriver.java
   - Refactoring after method renaming in AstManager, javadoc and isEmpty() usage.
10. src/com/goldencode/p2j/convert/TransformDriver.java
    - Removed unused ExcludeCheck and ScanCheck classes, javadoc, isEmpty() usage, refactoring after method renaming.
    - Process ArtifactConversionResult.
11. src/com/goldencode/p2j/convert/db/ConversionData.java
    - Change to support the artifact from ClassDefinition. TODO: Debug, I don't agree with using getName() in this case.
12. src/com/goldencode/p2j/schema/P2OLookup.java
    - Change to support the artifact from ClassDefinition. TODO: Use the artifact instead of the parentFile in the P2OLookup constructor.
13. src/com/goldencode/p2j/schema/SchemaLoader.java
    - Use the artifact in the ProgressLexer constructor.
14. src/com/goldencode/p2j/schema/SchemaWorker.java
    - Use the artifact from the ClassDefinition instance. TODO: Do not adjust the filename, create another artifact.
15. src/com/goldencode/p2j/uast/AstGenerator.java
    - Added artifact locking.
    - Replaced a few filename usages with artifacts.
    - Removed removeDuplicate()
    - Use artifact for the ProgressLexer/LexerDumpFilter.
    - TODO: Remove ArtifactManager.getArtifactByName(filename) calls, and use an artifact for markOriginal().
16. src/com/goldencode/p2j/uast/ClassDefinition.java
    - Replaced usage of filename with an artifact.
17. src/com/goldencode/p2j/uast/JavaClassDefinition.java
    - Support filename replacement.
18. src/com/goldencode/p2j/uast/LexerDumpFilter.java
    - Added constructors for artifacts.
19. src/com/goldencode/p2j/uast/SymbolResolver.java
    - Replaced filename usages with artifact.
    - Make use of the artifact from ClassDefinition where possible.
    - Fixed a regression related to DIR_CLASSES.
    - Fix an issue with empty maps (should not skip all of them when finding an empty one)
    - TODO: There's a scenario with propath that gave me a hard time, I have to investigate more - The idea is that sometime the path should not be removed from the relative name of the artifact and this issue has to do with the created artifacts.
20. src/com/goldencode/p2j/uast/Variable.java
    - Used the artifact from the ClassDefinition instance instead of the filename.
21. src/com/goldencode/p2j/uast/progress.g
    - Added getFilename() and getArtifact() to the lexer and parser and made use of the artifact where possible.

The more I did deeper into AstManager/AstGenerator, I see that we can also provide an id instead of the filename for similar operations. Greg, with the introduction of artifacts, should I also try to rely less on this?

Methods like AstSymbolResolver.getAst(AstSymbolResolver resolver, Long id, boolean loadTree) are quite difficult to refactor.

The more I did deeper into AstManager/AstGenerator, I see that we can also provide an id instead of the filename for similar operations. Greg, with the introduction of artifacts, should I also try to rely less on this?

Our objective here is to eliminate direct usage of filenames to load or access artifacts. The AM will handle making lists of these based on inputs for driver programs and there will be times when we need to find and add/remove dependencies or otherwise modify artifact collections, based on filenames though I want to limit this as much as we can.

But with things like the ASTs, the ID is absolutely critical and cannot be eliminated.

Methods like AstSymbolResolver.getAst(AstSymbolResolver resolver, Long id, boolean loadTree) are quite difficult to refactor.

I just want to elminate direct filename processing here. The ID processing should be left alone and should not be part of the artifacts.

Greg Shah wrote:

But with things like the ASTs, the ID is absolutely critical and cannot be eliminated.

Methods like AstSymbolResolver.getAst(AstSymbolResolver resolver, Long id, boolean loadTree) are quite difficult to refactor.

I just want to elminate direct filename processing here. The ID processing should be left alone and should not be part of the artifacts.

I understand. I was confused a bit because if I make this change I'd have to either lock based on the id (without knowing any filename) or simply lock the artifact. Thanks for the feedback.

Dănuț Filimon wrote:

I understand. I was confused a bit because if I make this change I'd have to either lock based on the id (without knowing any filename) or simply lock the artifact. Thanks for the feedback.

However, there's also the option of replacing the BasePlugin.fnames from a Map<String, Long> to a Map<Artifact, Long>, retrieving will be done based on the artifact.

After making lots of changes to replace the filename usage, I ended up changing some calls in the Worker classes to use Artifacts instead of the filename. This is not ok, as artifacts should be created at conversion and not in the rules. My idea here is to only create the artifact when the worker method is called and assume that the artifact was already created when starting the conversion.

However, there's also the option of replacing the BasePlugin.fnames from a Map<String, Long> to a Map<Artifact, Long>, retrieving will be done based on the artifact.

I like this.

After making lots of changes to replace the filename usage, I ended up changing some calls in the Worker classes to use Artifacts instead of the filename. This is not ok, as artifacts should be created at conversion and not in the rules. My idea here is to only create the artifact when the worker method is called and assume that the artifact was already created when starting the conversion.

Is this about intermediate or output artifacts that are being generated by rules? If so, then they do need to be created at that moment by the rules.

If this is about the Progress ASTs, then the associated artifact should already exist for each AST being walked by the pattern engine and we should expose that artifact to the workers as needed.

Greg Shah wrote:

After making lots of changes to replace the filename usage, I ended up changing some calls in the Worker classes to use Artifacts instead of the filename. This is not ok, as artifacts should be created at conversion and not in the rules. My idea here is to only create the artifact when the worker method is called and assume that the artifact was already created when starting the conversion.

Is this about intermediate or output artifacts that are being generated by rules? If so, then they do need to be created at that moment by the rules.

If this is about the Progress ASTs, then the associated artifact should already exist for each AST being walked by the pattern engine and we should expose that artifact to the workers as needed.

It's about worker methods that take a filename as a parameter, for example java_dmo.xml using JavaPatternWorker.persistJavaFile():

            <action>printfln("%s.jast", seqFile)</action>
            <rule>!java.persistJavaFile(sprintf('%s.jast', seqFile))
               <action>printfln("Failed to save JAST for %s", seqFile)</action>
            </rule>

I also think that the artifact should exist when the worker methods are called, but I've yet to run a hotel conversion with my latest changes.

I've got a question, if I have to create the artifact in the rules like in the following situation:

-                        <action>metaSchema = loadTree(metaDictFile)</action>
+                        <action>metaDictAr = art.addArtifact(metaDictFile)</action>
+                        <action>metaSchema = loadTree(metaDictAr)</action>

What is the best way to allow access to the ArtifactManager? Should I create a new Worker for that?

The idea is to create a new Worker that would provide functionalities to the ArtifactManager, I'll take a look at AbstractonversionWorker.java and see what needs to be done.

I ended up generating artifacts like this for ProgressASTs, so the issue from #7180-80 is valid. There are a few conversion issues with the latest changes and I am currently investigating using the hotel conversion.

The artifacts should still be stored in files with the same naming convention as currently used. We don't want to manufacture artificial names and then have to map them back to the original artifacts from which they came. That will be confusing and really hard to work with.

I've got a question, if I have to create the artifact in the rules like in the following situation:
[...] where art should be the ArtifactManager and metaDictAr should be an Artifact.

Shouldn't we do this the "other way around"? Get an artifact from the AM and then use that to load the AST.

What is the best way to allow access to the ArtifactManager? Should I create a new Worker for that?

Yes.

Greg Shah wrote:

I've got a question, if I have to create the artifact in the rules like in the following situation:
[...] where art should be the ArtifactManager and metaDictAr should be an Artifact.

Shouldn't we do this the "other way around"? Get an artifact from the AM and then use that to load the AST.

After investigating, I am on a situation from the rules which goes like this:

                           metaDictFile = schema.schemaConfig.getSchemaFile(metaName, ".dict").getPath()

But the situation above is not particular, there are many other cases where this can happen.

The overall answer is that the artifact will come from the AM in the end.

The problem is that I'll reach into NsConfig which I've already tried to refactor once and failed. I think I'll manage to do it now taking into account the progress of my changes.

I understand. This set of changes is very intrusive, but it does need to be that way. The changes here provide an important abstraction layer that will make the rest of the project cleaner.

Please do recall that we can subclass artifacts in "user" packages (like com.goldencode.p2j.schema or com.goldencode.p2j.pattern or com.goldencode.p2j.uast). Likewise we can provide a mechanism to register helper lambdas or register instances of some interface or provide a factor implementation... Use these mechanisms to provide some control to the user packages and avoid putting project-specific logic in the artifacts package.

Greg Shah wrote:

The problem is that I'll reach into NsConfig which I've already tried to refactor once and failed. I think I'll manage to do it now taking into account the progress of my changes.

I understand. This set of changes is very intrusive, but it does need to be that way. The changes here provide an important abstraction layer that will make the rest of the project cleaner.

Please do recall that we can subclass artifacts in "user" packages (like com.goldencode.p2j.schema or com.goldencode.p2j.pattern or com.goldencode.p2j.uast). Likewise we can provide a mechanism to register helper lambdas or register instances of some interface or provide a factor implementation... Use these mechanisms to provide some control to the user packages and avoid putting project-specific logic in the artifacts package.

I made a change and added a functional interface to be used in ArtifactsManager, but I have yet to expand it (only managed to reduce one scenario).

I committed 7180a/15854, it contains a big set of changes that replaces the usage of filenames in asts with artifacts and the necessary refactoring after this change.

The plan is to rebase to pick up #7255 (rather now than later when it becomes harder to do so) and then continue with fixing the conversion regressions I have introduced.

I have refactored the NsConfig to use an Artifact for the importFile and it worked out great, the only thing that needs to be mentioned here is that the configuration is read from an xml and the artifact will be created inside the setImportFile(String importFile) method. I also started converting the hotel project and addressing conversion problems related to methods that now use artifacts and I've added an ArtifactWorker for this.

1. In rules/schema/annotations.xml, where does the file come from? For example:
   

         <init-rules>
            <!-- load schema data associated with this AST -->
            <rule>schema.initializeDictionary(file)</rule>
         </init-rules>
   

file is a "global" variable that is provided by CommonAstSupport.

Current rebase issue.

ddf@ddfmachine:~/gcd/branches/7180a$ bzr rebase-continue
Committing to: /home/ddf/gcd/branches/7180a/                                                                                                                                           
modified src/com/goldencode/artifacts/Artifact.java
modified src/com/goldencode/artifacts/ArtifactCollection.java
modified src/com/goldencode/artifacts/ArtifactManager.java
modified src/com/goldencode/artifacts/BaseArtifact.java
modified src/com/goldencode/p2j/cfg/Configuration.java
modified src/com/goldencode/p2j/cfg/ProfileConfig.java
modified src/com/goldencode/p2j/pattern/ServiceSupport.java
modified src/com/goldencode/p2j/preproc/Options.java
modified src/com/goldencode/p2j/uast/CallGraphWorker.java
modified src/com/goldencode/p2j/uast/RootNodeList.java
Committed revision 15863.                                                                                                                                                              
Contents conflict in src/com/goldencode/p2j/uast/ScanDriver.java                                                                                                                       
1 conflicts encountered.                                                                                                                                                               
brz: ERROR: A conflict occurred replaying a commit. Resolve the conflict and run 'brz rebase-continue' or run 'brz rebase-abort'.
ddf@ddfmachine:~/gcd/branches/7180a$ bzr dif src/com/goldencode/p2j/uast/ScanDriver.java
=== renamed file 'src/com/goldencode/p2j/uast/ScanDriver.java' => 'src/com/goldencode/p2j/uast/ScanDriver.java.THIS' (properties changed: +x to -x)
ddf@ddfmachine:~/gcd/branches/7180a$ bzr status
added:
  src/com/goldencode/artifacts/ArtifactConversionResult.java
renamed:
  src/com/goldencode/p2j/uast/ScanDriver.java* => src/com/goldencode/p2j/uast/ScanDriver.java.THIS*
modified:
  src/com/goldencode/artifacts/Artifact.java
  src/com/goldencode/artifacts/ArtifactCollection.java
  src/com/goldencode/artifacts/ArtifactManager.java
  src/com/goldencode/artifacts/BaseArtifact.java
  src/com/goldencode/artifacts/SourceArtifact.java
  src/com/goldencode/ast/AstManager.java
  src/com/goldencode/p2j/convert/ConversionDriver.java
  src/com/goldencode/p2j/convert/TransformDriver.java
  src/com/goldencode/p2j/schema/SchemaLoader.java
  src/com/goldencode/p2j/uast/AstGenerator.java
  src/com/goldencode/p2j/uast/LexerDumpFilter.java
  src/com/goldencode/p2j/uast/progress.g
  src/com/goldencode/p2j/uast/ScanDriver.java.THIS*
unknown:
  src/com/goldencode/p2j/uast/ScanDriver.java.BASE
conflicts:
  Contents conflict in src/com/goldencode/p2j/uast/ScanDriver.java

Greg Shah wrote:

file is a "global" variable that is provided by CommonAstSupport.

Indeed, there is a getFile() method in CommonAstSupport which I can use. This will have to return an Artifact and be changed to getArtifact() eventually.

Question, did you unbind the branch first?

Constantin Asofiei wrote:

Question, did you unbind the branch first?

Yes.

Dănuț Filimon wrote:

Constantin Asofiei wrote:

Question, did you unbind the branch first?

Yes.

OK, then you will need to manually fix this - this is from a previous rebase? or something in 7180a, is not from trunk. Also, keep in mind that ScanDriver has recent trunk changes which would need to be integrated in 7180a.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin Asofiei wrote:

Question, did you unbind the branch first?

Yes.

OK, then you will need to manually fix this - this is from a previous rebase? or something in 7180a, is not from trunk. Also, keep in mind that ScanDriver has recent trunk changes which would need to be integrated in 7180a.

The issue is that I deleted ScanDriver.java in one of my commits, I am not sure if the renamed ScanDriver will be committed from what I can see. I am not sure how to solve this conflict, maybe bzr remove src/com/goldencode/p2j/uast/ScanDriver.java.THIS?

Dănuț Filimon wrote:

I am not sure how to solve this conflict, maybe bzr remove src/com/goldencode/p2j/uast/ScanDriver.java.THIS?

Yes, fix the current 'rebase state' of the branch, solve the conflicts and after that bzr rebase-continue should work.

• keep a copy of the 7170a branch as is, before rebasing; save a 'pref.diff' as bzr diff -r <trunk-rev> < ../prev.diff
• finish the rebase, do not push yet; save a 'current.diff' as bzr diff -r <trunk-rev> < ../current.diff

After that, do a meld prev.diff current.diff to check for any rebase mishaps; also, check 'ant compile' etc...

Constantin Asofiei wrote:

Dănuț Filimon wrote:

I am not sure how to solve this conflict, maybe bzr remove src/com/goldencode/p2j/uast/ScanDriver.java.THIS?

Yes, fix the current 'rebase state' of the branch, solve the conflicts and after that bzr rebase-continue should work.

But, please:

• keep a copy of the 7170a branch as is, before rebasing; save a 'pref.diff' as bzr diff -r <trunk-rev> < ../prev.diff
• finish the rebase, do not push yet; save a 'current.diff' as bzr diff -r <trunk-rev> < ../current.diff

After that, do a meld prev.diff current.diff to check for any rebase mishaps; also, check 'ant compile' etc...

Constantin, the patches will look different since I've made a lot of changes to a lot of places that were further modified in trunk. I will do as you say and if there is anything that needs to be fixed, I will do so in a separate commit.

The rebase is too problematic, I have to refactor changes that are not related to the commit in order to ensure compatibility.

I made the following patch for a quick fix on a function:

=== modified file 'rules/schema/dmo_common.rules'
--- old/rules/schema/dmo_common.rules    2025-02-18 11:48:12 +0000
+++ new/rules/schema/dmo_common.rules    2025-04-16 09:55:50 +0000
@@ -219,6 +219,8 @@
  -->

 <rule-set>
+   <worker class="com.goldencode.artifacts.ArtifactWorker"               namespace="art" />
+
    <!-- included function libraries -->
    <include name="common-progress" />

@@ -637,11 +639,15 @@
          <variable  name="varRoot"           type="com.goldencode.ast.Aast" />
          <variable  name="methRoot"          type="com.goldencode.ast.Aast" />
          <variable  name="legacyTable"       type="java.lang.String" />
+         <variable  name="fileArtifact"      type="com.goldencode.artifacts.Artifact" />

          <return    name="classAst"          type="com.goldencode.ast.Aast" />

          <!-- create java file; generates AST root for java class -->
-         <rule>!java.createJavaFile(filename, true, isRuntimeConfig())
+         <rule>fileArtifact = art.createArtifact(filename)</rule>
+         <rule>!java.createJavaFile(fileArtifact, true, isRuntimeConfig())
             <action>throwException(sprintf('Error creating java AST:  %s', filename))</action>
          </rule>

[java] ./cvt/data/hotel.p2o
     [java] Elapsed job time:  00:00:00.087
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] classAst = execLib('createCompileUnit', javaFile, package, ifaceName, #(boolean) parent.getAnnotation('permanent'), "", this.isAnnotation('include_oo'))
     [java]            ^  { Expression error [fileArtifact = art.createArtifact(filename)] }
     [java] ---------------------------
     [java] ERROR:
     [java] com.goldencode.p2j.pattern.TreeWalkException: ERROR!  Active Rule:
     [java] -----------------------
     [java]       RULE REPORT      
     [java] -----------------------
     [java] Rule Type :   WALK
     [java] Source AST:  [ GuestIdType ] DATA_MODEL/CLASS/ @0:0 {579820585134}
     [java] Copy AST  :  [ GuestIdType ] DATA_MODEL/CLASS/ @0:0 {579820585134}
     [java] Condition :  fileArtifact = art.createArtifact(filename)
     [java] Loop      :  false
     [java] --- END RULE REPORT ---

I discussed with Alex, #7180-101 is fixed by adding the worker in a file that includes the dmo_common.rules. I added the worker to all files, so I don't know which one exactly is responsible for it :)

I our last meeting I mentioned that I modified Aast.getFilename() to Aast.getArtifact(). filename is used a lot in the rules and I am thinking of leaving the getFilename() definition as it is and ensure is calls getArtifact().getRelativePath().

Dănuț Filimon wrote:

I our last meeting I mentioned that I modified Aast.getFilename() to Aast.getArtifact(). filename is used a lot in the rules and I am thinking of leaving the getFilename() definition as it is and ensure is calls getArtifact().getRelativePath().

As a first pass, that is OK. Ultimately, we do want to reduce or eliminate the dependency upon filenames. Examples of cases where we do need to still process filenames would be include files in the preprocessor, class name lookups in SR, name conversion for external procedures and classes.

But if we are using a filename for internal purposes and there is no 4GL compatibility or conversion requirement, then we probably want to move away from the filename usage.

Greg Shah wrote:

Dănuț Filimon wrote:

I our last meeting I mentioned that I modified Aast.getFilename() to Aast.getArtifact(). filename is used a lot in the rules and I am thinking of leaving the getFilename() definition as it is and ensure is calls getArtifact().getRelativePath().

As a first pass, that is OK. Ultimately, we do want to reduce or eliminate the dependency upon filenames. Examples of cases where we do need to still process filenames would be include files in the preprocessor, class name lookups in SR, name conversion for external procedures and classes.

But if we are using a filename for internal purposes and there is no 4GL compatibility or conversion requirement, then we probably want to move away from the filename usage.

I've already refactored the getFilename() usage since I replaced it with getArtifact() once so there is nothing to worry internally, the problem is that I have no way of perfectly verifying all usages of the filename in the rules (except direct search). This can do for the moment, but filename will have to go eventually.

I've went through the changes required to successfully convert the hotel gui project, the next step is to verify any of the calls I've changed in the rules and are not actually hit during conversion (e.g. I've changed persist() to use an artifact, there's another call in the rules that does not end up being used in the hotel conversion), cleanup the changes and commit the next iteration.

I've also setup ETF and plan to run a conversion with this project next.

Committed 7180a/15855. Expanded artifact usage to TRPL (initial).

• includeDictionary() was refactored to use an artifact instead of file.
• persist() calls now use artifacts.
• createJavaFile() calls now use artifacts.
• persistJavaFile() calls now use artifacts.

I made some changes to Aast and CommonAastSupport to allow filename and file to be used until I can remove them for good, but it mostly relies on the artifact itself which will make it easier when I'll make the change.

The rest of the changes include normal refactoring after modifying methods to use artifacts instead of the filename. The method naming remained the same, but I plan to change that soon.

7180a/15856 gets rid of the getFilename() and this.filename usage in TRPL for good. It was easier to investigate and commit this since the changes were clear.

Dănuț Filimon wrote:

7180a/15856 gets rid of the getFilename() and this.filename usage in TRPL for good. It was easier to investigate and commit this since the changes were clear.

I missed some scenarios and fixed those in 7180a/15857.

Greg, I ended up modifying rules/dbref/collect_refs.xml when removing file usage. It looks like both collect_refs.xml and apply_refs.xml are no longer used, so I plan to remove them.

Committed 7180a/15858. Removed getFile() from CommonAstSupport.java, modified rules accordingly.

Dănuț Filimon wrote:

Greg, I ended up modifying rules/dbref/collect_refs.xml when removing file usage. It looks like both collect_refs.xml and apply_refs.xml are no longer used, so I plan to remove them.

OK

7180a/15859 contains a few changes to methods that needed to be renamed and a few small changes to parameter names.

Next plan is to convert ETF/ChUI (small projects) to identify any issues before asking for a review.

ETF: There's a scenario where CommonAstSupport$Library.getPersistenceFilename() is still used (note: I removed since I thought it is not used, but I missed one). I will change it to getPersistenceArtifact() and update the rules.

ETF issue:

Elapsed job time:  00:00:02.678
ERROR:
com.goldencode.p2j.pattern.TreeWalkException: ERROR!  Active Rule:
-----------------------
      RULE REPORT      
-----------------------
Rule Type :   INIT
Source AST:  [ block ] BLOCK/ @0:0 {12884901889}
Copy AST  :  [ block ] BLOCK/ @0:0 {12884901889}
Condition :  file.endsWith(fixupFileName("<filename_from_the_etf_project>"))
Loop      :  false
--- END RULE REPORT ---

    at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1108)
    at com.goldencode.p2j.convert.TransformDriver.processTrees(TransformDriver.java:562)
    at com.goldencode.p2j.convert.ConversionDriver.back(ConversionDriver.java:575)
    at com.goldencode.p2j.convert.TransformDriver.executeJob(TransformDriver.java:911)
    at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1327)
Caused by: com.goldencode.expr.ExpressionException: Expression error [file.endsWith(fixupFileName(<filename_from_the_etf_project>))]
    at com.goldencode.expr.Expression.getCompiledInstance(Expression.java:695)
    at com.goldencode.expr.Expression.execute(Expression.java:387)
    at com.goldencode.p2j.pattern.Rule.apply(Rule.java:500)
    at com.goldencode.p2j.pattern.RuleContainer.apply(RuleContainer.java:597)
    at com.goldencode.p2j.pattern.RuleSet.apply(RuleSet.java:98)
    at com.goldencode.p2j.pattern.PatternEngine.apply(PatternEngine.java:1680)
    at com.goldencode.p2j.pattern.PatternEngine.processAst(PatternEngine.java:1587)
    at com.goldencode.p2j.pattern.PatternEngine.processAst(PatternEngine.java:1520)
    at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1071)
    ... 4 more
Caused by: com.goldencode.expr.CompilerException: Error parsing expression
    at com.goldencode.expr.Compiler.process(Compiler.java:378)
    at com.goldencode.expr.Compiler.compile(Compiler.java:298)
    at com.goldencode.expr.Expression.getCompiledInstance(Expression.java:687)
    ... 12 more
Caused by: com.goldencode.expr.UnresolvedSymbolException: No function resolved for instance (null) and variable (setter == false, name == (file))
    at com.goldencode.expr.ExpressionParser.method(ExpressionParser.java:1900)
    at com.goldencode.expr.ExpressionParser.primary_expr(ExpressionParser.java:847)
    at com.goldencode.expr.ExpressionParser.un_expr(ExpressionParser.java:1629)
    at com.goldencode.expr.ExpressionParser.prod_expr(ExpressionParser.java:1481)
    at com.goldencode.expr.ExpressionParser.sum_expr(ExpressionParser.java:1409)
    at com.goldencode.expr.ExpressionParser.shift_expr(ExpressionParser.java:1330)
    at com.goldencode.expr.ExpressionParser.compare_expr(ExpressionParser.java:1242)
    at com.goldencode.expr.ExpressionParser.equality_expr(ExpressionParser.java:1169)
    at com.goldencode.expr.ExpressionParser.bit_and_expr(ExpressionParser.java:1118)
    at com.goldencode.expr.ExpressionParser.bit_xor_expr(ExpressionParser.java:1073)
    at com.goldencode.expr.ExpressionParser.bit_or_expr(ExpressionParser.java:1028)
    at com.goldencode.expr.ExpressionParser.log_and_expr(ExpressionParser.java:983)
    at com.goldencode.expr.ExpressionParser.expr(ExpressionParser.java:931)
    at com.goldencode.expr.ExpressionParser.expression(ExpressionParser.java:680)
    at com.goldencode.expr.Compiler.parse(Compiler.java:482)
    at com.goldencode.expr.Compiler.process(Compiler.java:348)
    ... 14 more

I can't find the problematic condition is the rules, it looks like this is generated but I am not sure how.

There shouldn't be any TRPL code generated on the fly. I don't think we do that anywhere.

Are there customer-specific rule-sets in use?

Greg Shah wrote:

There shouldn't be any TRPL code generated on the fly. I don't think we do that anywhere.

Are there customer-specific rule-sets in use?

This is the case, etf defines two customer specific rules in a pattern folder. I wasn't even aware that this is possible, I'll have to fix those rules to proceed. Thank you!

By the way, is there any documentation on how those rules can be set in the project?

See Integrating Custom Conversion Logic.

The ChUI regression tests also have these customer-specific rule-sets.

Noted, I will check those when I'm done with ETF.

ETF custom rules only required a small change, only for the problematic line mentioned. I've started another conversion.

• Wrong absolute path for standard.dict when creating the artifact, the path shows the ./data/standard.dict in the server folder. This happens in SchemaConfig.getSchemaResource().
• org.h2.jdbc.JdbcSQLDataException: Invalid value "en_US_P2J" for parameter "collation"; SQL statement: set collation "en_US_P2J" [90008-200]

Currently investigating.

The issue with the SchemaConfig.getSchemaResource() is that the relative path of the artifact ./data/standard.dict can't be found by the MultiClassLoader.getClassLoader().getResource(<>) call, but data/standard.dict can be found in this case. The solution is to add an Artifact method getResourcePath() to strip the ./ from the relative path.

At the moment, this will avoid the original issue. I'd still like to fix the absoluteFilename call to use the correct home path when creating the artifact.

During the execution of ant import.db, the home will be /home/ddf/gcd/hotel_gui/, but when executing the server it will be /home/ddf/gcd/hotel_gui/deploy/server/ which is not ok. Artifacts should be built based on the project root, but the P2J_HOME is . (fwd.home) which allows this to happen (the default is also .).

Also, make sure we take into consideration that this location can be relative to the main project or to the conversion output directory (configurable but defaults to ./cvt/). This is the subject of the changes from #6202 and #5586.

At runtime, artifacts only should come from the application jar file. We should never be looking in the filesystem.

Greg Shah wrote:

At runtime, artifacts only should come from the application jar file. We should never be looking in the filesystem.

Yes, this should work fine in this case. But the implementation is conflicting with the ant import.db command. The error:

     [java] Using unnamed schema profile.
     [java] Elapsed job time:  00:00:02.516
     [java] ERROR:
     [java] java.lang.RuntimeException: ERROR!  Active rule could not be determined: Error loading configuration: schema/import; XML path: /cfg/worker
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1117)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.main(PatternEngine.java:2189)
     [java] Caused by: com.goldencode.p2j.cfg.ConfigurationException: Error loading configuration: schema/import; XML path: /cfg/worker
     [java]     at com.goldencode.p2j.pattern.ConfigLoader.loadImpl(ConfigLoader.java:651)
     [java]     at com.goldencode.p2j.pattern.ConfigLoader.load(ConfigLoader.java:552)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.initialize(PatternEngine.java:1208)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1043)
     [java]     ... 1 more
     [java] Caused by: com.goldencode.p2j.cfg.ConfigurationException: Unable to register worker: com.goldencode.p2j.schema.SchemaWorker
     [java]     at com.goldencode.p2j.pattern.ConfigLoader.worker(ConfigLoader.java:823)
     [java]     at com.goldencode.p2j.pattern.ConfigLoader.processChildElements(ConfigLoader.java:774)
     [java]     at com.goldencode.p2j.pattern.ConfigLoader.loadImpl(ConfigLoader.java:621)
     [java]     ... 4 more
     [java] Caused by: java.lang.reflect.InvocationTargetException
     [java]     at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
     [java]     at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
     [java]     at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
     [java]     at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
     [java]     at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
     [java]     at com.goldencode.p2j.pattern.RuleContainer.registerWorker(RuleContainer.java:687)
     [java]     at com.goldencode.p2j.pattern.ConfigLoader.worker(ConfigLoader.java:818)
     [java]     ... 6 more
     [java] Caused by: com.goldencode.p2j.schema.SchemaException: Error loading schema data
     [java]     at com.goldencode.p2j.schema.SchemaLoader.loadAst(SchemaLoader.java:899)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.loadSchema(SchemaLoader.java:826)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.loadDefaults(SchemaLoader.java:714)
     [java]     at com.goldencode.p2j.schema.SchemaDictionary.<init>(SchemaDictionary.java:1086)
     [java]     at com.goldencode.p2j.schema.SchemaDictionary.<init>(SchemaDictionary.java:1015)
     [java]     at com.goldencode.p2j.schema.SchemaWorker.<init>(SchemaWorker.java:224)
     [java]     ... 13 more
     [java] Caused by: com.goldencode.ast.AstException: Error loading AST XML file/resource: ./data/standard.dict
     [java]     at com.goldencode.ast.XmlFilePlugin.loadTree(XmlFilePlugin.java:470)
     [java]     at com.goldencode.ast.AstManager.loadTree(AstManager.java:343)
     [java]     at com.goldencode.p2j.schema.SchemaLoader.loadAst(SchemaLoader.java:884)
     [java]     ... 18 more
     [java] Caused by: java.lang.IllegalArgumentException: Missing/bogus input file/resource ./data/standard.dict
     [java]     at com.goldencode.ast.XmlFilePlugin.loadTree(XmlFilePlugin.java:463)
     [java]     ... 20 more

The MultiClassLoader.getClassLoader().getResource(<>) finds the artifact, but when loading it useResources is actually false. This doesn't look right.

I also have a question, where should fwd_sheet.war be built? It seems that it's missing from the library in the hotel project and is also an issue when starting the server. I need to know more because I can't identify what is missing.

This doesn't look right.

Correct. We should not use the filesystem during import.

Greg Shah wrote:

This doesn't look right.

Correct. We should not use the filesystem during import.

XmlFilePlugin is initialized before the ImportWorker which sets the Configuration.setImportMode();, this is the issue.

Dănuț Filimon wrote:

I also have a question, where should fwd_sheet.war be built? It seems that it's missing from the library in the hotel project and is also an issue when starting the server. I need to know more because I can't identify what is missing.

This is an optional target added to the FWD build as in ./gradlew core sheet:war.

Greg Shah wrote:

Dănuț Filimon wrote:

I also have a question, where should fwd_sheet.war be built? It seems that it's missing from the library in the hotel project and is also an issue when starting the server. I need to know more because I can't identify what is missing.

This is an optional target added to the FWD build as in ./gradlew core sheet:war.

I see, thanks!

As for the import issue, it's because I moved a static block in AstManager I brought from the ScanDriver. After removing it, I progressed to the next error (not finding the files for import).

Committed 7180a/15860.

• Added getResourcePath() to artifacts - used to get the resource path for the MultiClassLoader.
• There's a ./null.p2o file being searched at server startup, had to add a null check. The issue is also present in trunk.
• Added getPersistenceFilename() - for conversion, I need to remove it. Maybe using an artifact method ~ getPersistentArtifact()
• Removed the static block from AstManager because it was loaded too early during database import.

I've fixed the ETF project and managed to run the server (it wasn't just 7180a issues), currently running a test and I don't see any errors in the server.log.

Added getPersistenceFilename() - for conversion, I need to remove it. Maybe using an artifact method ~ getPersistentArtifact()

Not sure about this. I think this needs to be a generic solution, not one written to this case. This case is the AST name derived from the original source artifact filename. So it is a way to get the AST artifact from the source artifact. We should provide some linkage between source and intermediate/output artifacts. Access to one of these should allow the caller to traverse to the other (intermediate/output to source OR source to intermediate/output). This will also be needed for operations like deleting intermediate/output artifacts associated with a given list of source artifacts.

getPersistenceFilename() existed before, but I removed it because I thought it wasn't used. I do agree that I should have a separate artifact for the .ast, but I first need to find a way to link those artifacts without relying on creating a new artifact. The .ast artifact should exist for each defined artifact at some point, doing it earlier might be better as we do not need to check if it already exists (since the original did not).

getPersistenceFilename() existed before, but I removed it because I thought it wasn't used.

It is used as persistenceFilename from post-parse fixups.

Eduard, can you take the patch from #7180-139, patch it to trunk and test the scenarios you've created for #7255 (trunk/15852)? I had to refactor that revision to fit with my changes, so I want to make sure everything is correct.

I had to add a few methods to ArtifactCollection to make it compatible with that trunk revision and I might attempt another rebase now that I got it to work. Any new methods added can just go into a separate commit.

Dănuț Filimon wrote:

Eduard, can you take the patch from #7180-139, patch it to trunk and test the scenarios you've created for #7255 (trunk/15852)? I had to refactor that revision to fit with my changes, so I want to make sure everything is correct.

I had to add a few methods to ArtifactCollection to make it compatible with that trunk revision and I might attempt another rebase now that I got it to work. Any new methods added can just go into a separate commit.

The propath entries are not correctly set in SymbolResolver.createClassMappings.

I think you should use String absoluteName = a.getAbsolutePath(); instead of String relativeName = a.getRelativePath();, and use absoluteName to calculate the entry.

Danut/Eduard: note that in trunk rev 15888.1.1 (commit 15889) there were some other fixes related to #7255.

Constantin Asofiei wrote:

Danut/Eduard: note that in trunk rev 15888.1.1 (commit 15889) there were some other fixes related to #7255.

I'll make a new patch and post an updated version.

I have been trying the test with 7180a-15894.patch, the following test case:

FooTest.cls:

class oo.FooTest inherits oo.BarTest:
end.

BarTest.cls:

class oo.BarTest:
end.

In file-cvt-list.txt only include FooTest.cls.

One thing to note in ScanDriver.scan, in job lambda do not try to get the artifact from ArtifactCollection Artifact a = artifacts.get(i), use instead the artifact obtained from the iterator Artifact artifact = iter.next();.

But in any case the conversion is broken for this example at Code Conversion Annotations phase, I don't know if it is related to #7255 in any way (because without file-cvt-list.txt I get the same results).

Rebased 7180a to latest trunk/15894, the branch is now at revision 15908.

The patch helped a lot, I had to bzr revert the ScanDriver during the rebase o avoid any issues. I will follow up with a commit to address the required methods for the branch to be built, including #7180-141 and #7180-146.

Thanks Eduard for taking a look at the changes.

Eduard Soltan wrote:

I have been trying the test with 7180a-15894.patch, the following test case:

FooTest.cls:
[...]

BarTest.cls:
[...]

In file-cvt-list.txt only include FooTest.cls.

One thing to note in ScanDriver.scan, in job lambda do not try to get the artifact from ArtifactCollection Artifact a = artifacts.get(i), use instead the artifact obtained from the iterator Artifact artifact = iter.next();.

The problem is actually different, I already have a call to iter.next() at the start of the for loop so executing another iter.next() in the lambda will skip elements from the list and result in a NoSuchElementException. The solution is simple, either delete the get call and use the artifact from the first iter.next() or use the existent get call - I chose to remove the get() call.

But in any case the conversion is broken for this example at Code Conversion Annotations phase, I don't know if it is related to #7255 in any way (because without file-cvt-list.txt I get the same results).

Using your example I found an issue in rules/annotations/method_defs.rules because of this rule:

action>ref = loadTree(sprintf("%s.ast", getNoteString("source-file")))</action>

7180a/15909 contains the changes mentioned in #7180-148, including the compatibility changes for the trunk rebase. The branch can be built now.

Committed 7180a/15910. Renamed methods, fixed javadoc. Also found a few issues which had to be addressed, then found ways to use artifacts more in some cases.

I plan to get rid of plain ".ast" or ".dict" usages and turn them into ArtifactManager variables to be used in my next commit. Maybe I'll be able to find more scenarios I can improve this time too.

I plan to get rid of plain ".ast" or ".dict" usages and turn them into ArtifactManager variables to be used in my next commit. Maybe I'll be able to find more scenarios I can improve this time too.

These are things that should be in the FWD code (com.goldencode.p2j.*) not in the "generic" AM code (com.goldencode.artifacts). The AM on its own should not know about specific file extensions.

Greg Shah wrote:

I plan to get rid of plain ".ast" or ".dict" usages and turn them into ArtifactManager variables to be used in my next commit. Maybe I'll be able to find more scenarios I can improve this time too.

These are things that should be in the FWD code (com.goldencode.p2j.*) not in the "generic" AM code (com.goldencode.artifacts). The AM on its own should not know about specific file extensions.

I understand, what about creating a class in com.goldencode.p2j.cfg.*? I want to keep track of all extensions used and not have them all over the place.

Dănuț Filimon wrote:

Greg Shah wrote:

I plan to get rid of plain ".ast" or ".dict" usages and turn them into ArtifactManager variables to be used in my next commit. Maybe I'll be able to find more scenarios I can improve this time too.

These are things that should be in the FWD code (com.goldencode.p2j.*) not in the "generic" AM code (com.goldencode.artifacts). The AM on its own should not know about specific file extensions.

I understand, what about creating a class in com.goldencode.p2j.cfg.*? I want to keep track of all extensions used and not have them all over the place.

OK

I committed the #7180-151 - #7180-154 changes to 7180a/15911.

Committed 7180a/15912. Missed conversion changes for the #7180-151 commit, getSchemaFile() and getSchemaFileName() were renamed to getSchemaArtifact().

9408a was merged to trunk, I am planning a rebase because the branch is affected. See #9408-21.

Rebased 7180a to latest trunk/15918, the branch is now at revision 15936.

I also committed changes to src/com/goldencode/p2j/convert/GenerateLegacyProxyOpenClient.java, making the proxyFiles an ArtifactCollection and rewriting some of the functionality. The revision is 7180a/15937. Constantin, can you take a look at the last revision and let me know if there is anything I need to change? During rebase I also replaced the usages of file in rules that you added with artifact.getRelativePath(), so no need to worry about the rules.

I've done an ETF conversion with 7180a/15912 (before rebase) and it was successful, I'll start runtime tests and check if there are any issues.

Dănuț Filimon wrote:

The revision is 7180a/15937. Constantin, can you take a look at the last revision and let me know if there is anything I need to change?

Rev 15937 looks OK.

Rebased 7180a to latest trunk/15932, the branch is now at revision 15951.

I was going into the customer rules for the ChUI project and noticed a getSourceFor() call which returns a filename. IncludeHint, UastHints and PreprocessorHints can be refactored to use artifacts.

This will be my target for the next commit :)

One issue with using artifacts in UastHints which is problematic. The example is AstGenerator.processFile() where we lock the artifact and create new artifact and we keep processing the filename, eventually creating new artifacts... I want to avoid creating an artifact where the filename matches the original and we end up with the locked artifact.

Holding the changes for a bit until I give some thought to this change, I want to avoid this potential issue and maybe rethink AstGenerator.processFile().

I decided that I will hold off the changes to UastHints due to #7180-163, I shifted my focus to getting rid of the FileOperationsWorker dependency in the ArtifactManager and cleaning up any unused/unnecessary methods.

Committed 7180a/15952. Simplified transformArtifactNames() and removed containsArtifact() from ArtifactManager.

• transformArtifactNames() change required to remove com.goldencode.p2j.pattern.FileOperationsWorker dependency.
• containsArtifact() has an alternative in ArtifactCollection.

Rebased 7180a to latest trunk/16090, the branch is now at revision 7180a/16110. I will follow up with another commit since the branch isn't building anymore, I missed a few imports during rebase and there are also a few changes that are not compatible with 5586a and need some work.

1. A source artifact is transformed into an artifact so the folder will change from ./abl/ to ./cvt/, this is not an issue as transforming any source artifact can use the Configuration.getPathToConversionFolder() to get the correct path of the new artifact.
2. There is a method in ArtifactManager that retrieves the original artifact (so from .p.ast, it gets the .p artifact), this will have to use Configuration.getPathToSourceFileFromConversionFolder().

Conversion is broken at the moment, but I have all the resources needed to fix it.

There are also cases where I create an artifact by doing artifact.getRelativePATH() + the extension, those should also rely on Configuration.getPathToConversionFolder(). I am patching the problems and plan to refactor the ArtifactManager for such cases.

Please consider if code like Configuration.getPathToSourceFileFromConversionFolder() should be in the AM instead of in Configuration. Perhaps the Configuration class should set the configuration values into the AM and then the AM would do the calculation of the various paths.

Greg Shah wrote:

Please consider if code like Configuration.getPathToSourceFileFromConversionFolder() should be in the AM instead of in Configuration. Perhaps the Configuration class should set the configuration values into the AM and then the AM would do the calculation of the various paths.

I will look into it, this idea is very good. I want to avoid the Configuration dependency in the ArtifactManager.

Committed 7180a/16111. Compatibility changes after rebasing and picking up 5586a.

I will also look into #7180-171 and make a separate commit.

• getPathToConversionFolder(): basepath parameter, Configuration.DEFAULT_CVT, the conversion folder from the getConversionFolder()
• getPathToSourceFileFromConversionFolder(): similar to the previous method

I added three parameters which are initialized in a private ArtifactManager constructor, then I moved getPathToConversionFolder and getPathToSourceFileFromConversionFolder to the AM with no issues. I'll modify the rest of the calls to use the methods from the AM and do a small conversion (hotel), then ETF. I also have to rebase to pick up the recent fixes for trunk/16089 (trunk/16107 and trunk/16109).

Rebased 7180a to latest trunk/16110, the branch is now at revision 16131.

Committed 7180a/16132: Moved getPathToConversionFolder() and getPathToSourceFileFromConversionFolder() to ArtifactManager.java.

Code Review Task Branch 7180a Revisions 16121 through 16139

This is really good. We are getting close.

1. annotations/method_defs.rules explicitly creates the artifact (i.e. calls art.createArtifact(filename)). Shouldn't loadTree() handle this?

2. convert/proxy_prgrams.xml and similar cases (schema/java_dmo.xml ...) have the same issue with createArtifact(). Shouldn't the createJavaFile() handle this? This would also affect locations calling execLib('createCompileUnit', ....

3. dbSchemaPath = schemaConfig.getSchemaArtifact(dbname, ".schema") in callgraph/events.rules is a confusing name. Instead of an Artifact it returns a String.

4. In the ArtifactManager, the conversion folder should be called the project folder. The AM will be used for more than just conversion (e.g. full IDE support and many other tools). All method names and data members using the conversion folder naming, should be changed. Any reference to conversion, even createConversionArtifacts() should be made more generic.

5. The AM.addLexerArtifact() methods seem too specific to FWD. How do we make these generic or allow FWD to specialize this processing without hard coding it into the AM?

6. The BaseArtifact.mustConvert() method and its use of ConversionData are too specific to FWD. How do we make these generic or allow FWD to specialize this processing without hard coding it?

7. In multiple classes we now have fartifact or fartifacts as a name. In English "fart" has a humorous meaning. A fartifact reads as a bit of a silly name as a result. We should probably change that.

8. In ConversionDriver, you changed one 0 to an O in brew(pojoArtifacts, "Plain 0ld Java Objects (P0J0s)"); but there are still 3 more 0 chars. Let's fix that now even though you didn't create the problem.

9. We should remember that we still have to remove the dependencies upon com.goldencode.p2j.cfg.* from com.goldencode.artifacts.*.

10. I think the computing of file-level hashes in ConversionData should be in the Artifact.

Greg Shah wrote:

Code Review Task Branch 7180a Revisions 16121 through 16139

I plan to rebase and address a few conflicts, 7180a is based on trunk/16115 so there are a lots of changes that will need to be fixed.

1,2 - There are definitely some cases where I was concerned that some artifacts do not exist yet, so whenever I saw we build a new name using extensions, I just created a new artifact. This is also because I lacked tests to check those areas for the artifacts, but I am sure that the artifacts exist in those cases.
3 - schemaConfig.getSchemaArtifact(dbname, ".schema") returns an Artifact, dbSchemaPath has the wrong type.
4 - I'll fix this (conversion -> project, or other name depending on the usage).
5 - The issue originates from the Reader that the lexer artifact needs. I can remove the method and use addArtifact(), then just set the Reader in the same place I create the artifact.
6 - Pass the artifact through the AM and let the it call ConversionData.
7, 8 - Coming up with variable names is so hard sometimes, name = artifact was a quick move. I will also make sure to replace 0 with O.
9 - This is on my list of TODOs
10 - I remember trying to introduce artifacts to ConversionData and failing, it's worth trying again.

I managed to make 7180a build, but I don't like a few changes so I'll be working a bit on 7180a and also take care of as many points from #7180-177 as possible.

The following pattern:

            <action>filename = sprintf("%s.ast", getNoteString("source-file"))</action>
            <action>artifact = art.createArtifact(filename)</action>
            <action>ref = loadTree(artifact)</action>

• annotations/cross_namespace_conflicts.rules - used by persist()
• annotations/method_defs.rules - used by loadTree()
• convert/proxy_programs.xml - used by createJavaFile()
• include/common-progress.rules - used through a function by createJavaFile(), persistJavaFile
• schema/fixups.xml - used by brainwash()
• schema/iface_infra.rules - used by execLib('createCompileUnit',
• schema/java_dmo.xml - used by persistJavaFile() and createJavaFile()
• schema/java_pojo.xml - used by createJavaFile()
• schema/super_iface.rules - used by execLib('createCompileUnit',

Fixed number 3, 4, 5 (got rid of addLexerArtifact()), 7, 8 during rebase.

TODO: 1, 2, 6, 9, 10 left + cleanup

Greg, do you have any tips on how I can get rid of Configuration from the AM? I need to get some parameters like basepath, output-root, cvt and also need to use normalizeFilename(), absoluteFilename().

The following pattern:
[...] where an artifact is created to call loadTree() is because loadTree() takes an Artifact parameter instead of a filename now. I can add the loadTree(String) back, this will call the createArtifact and it will remove some of the changes.

Yes, do this.

Greg, do you have any tips on how I can get rid of Configuration from the AM? I need to get some parameters like basepath, output-root, cvt and also need to use normalizeFilename(), absoluteFilename().

I think the "user" code (FWD in this case) should initialize the AM with the proper project-level values. For facilities like normalizeFilename() and absoluteFilename(), I would expect that this code might be best if moved into the AM.

Fixed number 1, 2, 6.

I am a bit stuk at 9, getting the Configuration out of the AM. I have to move Configuration.getArtifactCol() to the AM, but it contains this call cfg.loadConfigProfilesImpl(defaultProfiles); which makes it difficult as I'll need a lot more parameters.

How it's initialized:

      ArtifactManager am = ArtifactManager.getInstance();
      String basepath = Configuration.getParameterAsPath("basepath");
      String outputRoot = Configuration.getParameterAsPath("output-root");
      String defaultProject = Configuration.DEFAULT_CVT;
      String projectFolder = Configuration.getConversionFolderAsPath();
      String home = null;
      try
      {
         home = Configuration.home();
      }
      catch (ConfigurationException e)
      {
         // nothing
      }
      am.setProperties(basepath, outputRoot, defaultProject, projectFolder, home);

Committed 7180a/16245. Addressed the rebase issues, compatibility changes and most of the review points from #7180-177.

I'll continue to work on the ConversionData changes, and also find an idea on removing Configuration.getArtifactCol() call from the AM.

Here goes another commit to 7180a/16246! Allow ConversionData to use artifacts, added computeHash() to Artifact, use artifacts for hints. It was much easier now to find the proper changes to ConversionData and Hints and use artifacts.

The only issue left is with removing Configuration from the AM and testing.

Dănuț Filimon wrote:

The only issue left is with removing Configuration from the AM and testing.

• Create a separate class that holds the values for ArtifactManager and Configuration
• Set the values for the AM in the Configuration when they are updated.

Rebased 7180a to latest trunk/16242, the branch is now at revision 16270.

Committed 7180a/16271 to make 7180a compatible with trunk.

• All artifacts were created as BaseArtifact, there's a few places that require SourceArtifact and there was a cast problem.
• JavaPatternWorker.storeJavaSourceFileReference() was using filename, which was replaced by artifact.
• Lots of FileNotFoundException aused by: java.io.FileNotFoundException: /home/ddf/gcd/branches/hotel_gui/./cvt/cvt/update-stay-dialog.w.cache (No such file or directory) because getPathToProjectFolder() was being called twice.
• Issue were both BaseArtifact and SourceArtifact are expected for the same file.
• JavaClassDefinition uses java.awt.Color as the filename and this is not compatible with the artifacts.

I committed the fixes to 7180a/16272.

Hotel is converting properly with this, I will also convert a few larger applications.

• warning: [removal] Integer(int) in Integer has been deprecated and marked for removal
• warning: [removal] Integer(String) in Integer has been deprecated and marked for removal
• warning: [removal] Long(long) in Long has been deprecated and marked for removal
• error: neutralMillisToCalendar(long,TimeZone) has private access in date
• error: julianDayToMillis(long) has protected access in date
• warning: [removal] Boolean(boolean) in Boolean has been deprecated and marked for removal

Dănuț Filimon wrote:

• error: neutralMillisToCalendar(long,TimeZone) has private access in date
• error: julianDayToMillis(long) has protected access in date

This patch fixes the issue and the converted project compiles:

=== modified file 'src/com/goldencode/p2j/util/date.java'
--- old/src/com/goldencode/p2j/util/date.java    2025-10-22 17:06:34 +0000
+++ new/src/com/goldencode/p2j/util/date.java    2025-10-28 09:32:43 +0000
@@ -2505,7 +2505,7 @@
     *
     * @return   The number of milliseconds since the Java epoch.
     */
-   protected static long julianDayToMillis(long jd)
+   public static long julianDayToMillis(long jd)
    {
       return (jd - JULIAN_EPOCH_OFFSET) * MILLIS_PER_DAY;
    }
@@ -4551,7 +4551,7 @@
     *           time-neutral result will be obtained when using the timezone
     *           aware resulting class.
     */
-   private static GregorianCalendar neutralMillisToCalendar(long millis, TimeZone zone)
+   public static GregorianCalendar neutralMillisToCalendar(long millis, TimeZone zone)
    {
       GregorianCalendar gc = null;

I am not sure if this is happening with the latest trunk or revision 16242 (7180a is based on it), so I'll have to test this out.

Dănuț Filimon wrote:

Dănuț Filimon wrote:

• error: neutralMillisToCalendar(long,TimeZone) has private access in date
• error: julianDayToMillis(long) has protected access in date

This patch fixes the issue and the converted project compiles:
[...] This however does not look like an issue introduced by 7180a.

I am not sure if this is happening with the latest trunk or revision 16242 (7180a is based on it), so I'll have to test this out.

Compile phase also fails with trunk/16242.

Code Review Task Branch 7180a Revisions 16267 through 16273

Really good. Minor feedback:

1. The gen_fname function in frame and menu generation can be common code (e.g. common-progress.rules where gen_fartifact used to be).

2. References to DMOs (e.g. JavaDoc for pkgRootPath) and Pojos (e.g. pojoRoot) don't belong in the ArtifactManager. Another example is getPathToProjectFolderPojoFromDmo(). The AM shouldn't have knowledge of FWD-specific stuff. If we need these in the short term, it is fine. I just want to make it clear that the objective here is to make the artifact processing generic/external to FWD.

3. Could loadTree(String filename) and getTreeId(String filename) be implemented as default methods in AstManagerPlugin instead of putting the code in the implementation classes?

4. Please use the wildcards for imports. This is the case for at least these: BaseArtifact, BasePlugin, InMemoryRegistryPlugin, NameMappingWorker, ConversionData, ClearStream.

Greg Shah wrote:

Really good. Minor feedback:
2. References to DMOs (e.g. JavaDoc for pkgRootPath) and Pojos (e.g. pojoRoot) don't belong in the ArtifactManager. Another example is getPathToProjectFolderPojoFromDmo(). The AM shouldn't have knowledge of FWD-specific stuff. If we need these in the short term, it is fine. I just want to make it clear that the objective here is to make the artifact processing generic/external to FWD.

I can just pass pkgrootpath and pojoroot as parameters from cleanupDeletedSchemaTables to getPathToProjectFolderPojoFromDmo.

3. Could loadTree(String filename) and getTreeId(String filename) be implemented as default methods in AstManagerPlugin instead of putting the code in the implementation classes?

Yes.

Committed 7180a/16274. Addressed the review.

I am working on a rebase. I will put the task into Internal Test if there are no major modifications required.

Rebased 7180a to latest trunk/16304, the branch is now at revision 16336.

The good news is that the only compatibility changes required are because of #4867.

Code Review Task Branch 7180a Revision 16336

Let's rename AM.getPathToProjectFolderPojoFromDmo() to something more generic. Otherwise it is good to go.

Greg Shah wrote:

Code Review Task Branch 7180a Revision 16336

Let's rename AM.getPathToProjectFolderPojoFromDmo() to something more generic. Otherwise it is good to go.

What about getJavaObjectPath() or getPojoPath()?

Dănuț Filimon wrote:

Greg Shah wrote:

Code Review Task Branch 7180a Revision 16336

Let's rename AM.getPathToProjectFolderPojoFromDmo() to something more generic. Otherwise it is good to go.

What about getJavaObjectPath() or getPojoPath()?

Java and POJOs are things that are specific to P2J. What is the method trying to do? Focus on the operation that is happening rather than the meaning of the parameters.

Greg Shah wrote:

Dănuț Filimon wrote:

Greg Shah wrote:

Code Review Task Branch 7180a Revision 16336

Let's rename AM.getPathToProjectFolderPojoFromDmo() to something more generic. Otherwise it is good to go.

What about getJavaObjectPath() or getPojoPath()?

Java and POJOs are things that are specific to P2J. What is the method trying to do? Focus on the operation that is happening rather than the meaning of the parameters.

It takes a DMO jast and finds the path to the pojo jast, the method is used when cleaning up deleted schema tables. I'll make it general and call it getSiblingPath() since the purpose of the method is to change a path to another (sibling) path.

I'll make it general and call it getSiblingPath() since the purpose of the method is to change a path to another (sibling) path.

Perfect!

• Found a loadTree() call that looks for an ast file in the abl folder.
• Found a scenario where the AM.fileSet is not set.
• ArtifactManager.processFileSet() was missing the filename processing.

• #10816- duplicate getOriginalArtifact() call - fixed
• #10613 - testing passed, no issues
• #7793 - dmo path was deleted instead of the pojo - fixed
• #6256 - fixed, mentioning a profile should not allow additional arguments, the ant commands will need to be fixed
• #10208 - currently working on this. The AM.homePath needs to be initialized when setting the import file.

• #6083 - working as expected
• #6738 - deleting artifacts didn't work (name_map.xml was not updated properly)
• #10265 - working as expected

I fixed #7180-206, #4867 and the issues above in 7180a/16339. I am doing another rebase.

the ant commands that use profile must be modified to exclude additional parameter. The purpose would be that a profile has all the necessary information to run the conversion and must not be combined with other modes. Greg, let me know if you agree.

Yes, agreed.

the import command must point to the .p2o file in the cvt folder, and not the ./data/ folder:

Yes, very much agreed.

Committed 7180a/16346. clean() used a file instead of the artifact, this was a rebase issue.

Code Review Task Branch 7180a Revisions 16356 and 16357

getConfigNameMapParameter() should be in NameMappingWorker instead of CommonAstSupport. CommonAstSupport is for generic features of TRPL, not conversion-specific stuff.

Otherwise the changes look fine. Make sure it works at runtime because the SourceNameMapper stuff is heavily used at runtime.

Greg Shah wrote:

Code Review Task Branch 7180a Revisions 16356 and 16357

getConfigNameMapParameter() should be in NameMappingWorker instead of CommonAstSupport. CommonAstSupport is for generic features of TRPL, not conversion-specific stuff.

Otherwise the changes look fine. Make sure it works at runtime because the SourceNameMapper stuff is heavily used at runtime.

I addressed this in 7180a/16359. Revision 16358 contains compatibility changes after the rebase (see #11030-8)

Fixed the issue mentioned in #11030-14 in 7180a/16360. Files were parsed by rules more than one time, there was also a .ast file that wasn't using the project folder path.

Code Review Task Branch 7180a Revisions 16358 through 16360

The changes look good.

Committed 7180a/16361. Replace getArtifact().getRelativePath() with getFilename() when using ClassDefinition of JavaClassDefinition. (refs: #7180)

See #11030-18

Code Review Task Branch 7180a Revision 16361

The changes look good.

Committed 7180a/16362. Initialize the ArtifactManager properties at server startup, before the SourceNameMapper.

See #11030-21

Code Review Task Branch 7180a Revisions 16410 through 16412

No objections.

Code Review Task Branch 7180a Revision 16413 and 16414

In the AM, let's change the name importMode to something more generic. The AM should have no concept of an "import". That is a FWD concept. You can refer to this as jarMode instead. Generically, reading things from a jar as a Java concept, not something specific to FWD.

Otherwise the changes are fine.

After retesting ChUI reports, I've found an interesting conversion issue which I am currently investigating. I also reworked the ArtifactManager properties initialization (having it spread out in multiple places is a bad practice).

Danut,
I have just updated FWD locally and, when connecting with a client I am getting this exception:

26/02/24 22:31:02.637+0200 |  SEVERE | com.goldencode.p2j.net.Dispatcher [Dispatcher.processInbound()] | ThreadName:Conversation [00000001:bogus], Session:00000001, ThreadId:00000004, User:bogus | Unexpected throwable.
java.lang.reflect.InvocationTargetException
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:569)
    at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:126)
    at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:808)
    at com.goldencode.p2j.net.Conversation.block(Conversation.java:418)
    at com.goldencode.p2j.net.Conversation.run(Conversation.java:237)
    at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: java.lang.NullPointerException: Cannot invoke "java.util.Set.contains(Object)" because "com.goldencode.p2j.util.SourceNameMapper.allowedLegacyExtensions" is null
    at com.goldencode.p2j.util.SourceNameMapper.fastConvertName(SourceNameMapper.java:2263)
    at com.goldencode.p2j.util.SourceNameMapper.fastConvertName(SourceNameMapper.java:2215)
    at com.goldencode.p2j.util.SourceNameMapper.convertNameToClass(SourceNameMapper.java:790)
    at com.goldencode.p2j.util.SourceNameMapper.convertNameToClass(SourceNameMapper.java:770)
    at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:634)
    ... 9 more

mappingDataInitInProgess is true, but mappingDataInitialized is false. The reason is the developer mode is not working as expected. The resource is null as returned by systemClassLoader.getResource(nameMappingXmlPath); and the method returns without any warning message. The initialization is incomplete. I think something is missing here.

To make it work on my developer environemnt, I injected

            if (resource == null)
            {
               try
               {
                  resource = new URL("file:../../cvt/" + nameMappingXmlPath); // developer mode
               }
               catch (MalformedURLException e)
               {
                  // silently ignore it
               }
            }

Greg,
I do not know if the above code can be used as it is. If that's not possible, I will have have to find some workaround.

I think we need to be able to configure this path at runtime in this developer mode. We can't assume it is in ../...

Danut: Please come up with a plan and a fix.

Greg Shah wrote:

I think we need to be able to configure this path at runtime in this developer mode. We can't assume it is in ../...

Danut: Please come up with a plan and a fix.

The problem is similar to the one found when trying to run the report server (#10919). The files are read using relative path and the working directory for the client (<home>/deploy/server/), we won't be able to find the files. Fixing this in the server.sh is what needs to be done.

If that is not feasible, I will create a sym link to the expected place for this file each time the IDE workplace is initialized. Note that, in developer mode, there is no p2j.jar and no app.jar, all files are not packed, in the locations where they were written (by conversion) initially.

Ovidiu Maxiniuc wrote:

If that is not feasible, I will create a sym link to the expected place for this file each time the IDE workplace is initialized. Note that, in developer mode, there is no p2j.jar and no app.jar, all files are not packed, in the locations where they were written (by conversion) initially.

How can I enable the developer mode?

Ovidiu Maxiniuc wrote:

If that is not feasible, I will create a sym link to the expected place for this file each time the IDE workplace is initialized. Note that, in developer mode, there is no p2j.jar and no app.jar, all files are not packed, in the locations where they were written (by conversion) initially.

The server.sh change might need a lot more changes, I've managed to replace the references and have the script run from the project_home. In the end, when it tried to load server.xml, it failed because of <xml filename="directory.xml"/> (it exists in deploy/server/).

Ovidiu Maxiniuc wrote:

Danut,
I have just updated FWD locally and, when connecting with a client I am getting this exception:
[...]The client is disconnected immediately.

mappingDataInitInProgess is true, but mappingDataInitialized is false. The reason is the developer mode is not working as expected. The resource is null as returned by systemClassLoader.getResource(nameMappingXmlPath); and the method returns without any warning message. The initialization is incomplete. I think something is missing here.

To make it work on my developer environemnt, I injected [...]before if (resource != null). The name_map.xml is generated in the cvt/ folder of the root of the project and the server executes from deploy/server/. In developer mode, the artefact files are not packed in jars and must be found on local file system. That's the reason for ../../cvt.

Greg,
I do not know if the above code can be used as it is. If that's not possible, I will have have to find some workaround.

Ovidiu, are you sure the

resource = systemClassLoader.getResource(nameMappingXmlPath);

Ovidiu, can you try the following and let me know if it works?

=== modified file 'deploy/server/server.sh'
--- old/deploy/server/server.sh    2026-02-16 17:50:07 +0000
+++ new/deploy/server/server.sh    2026-02-25 14:06:22 +0000
@@ -6,6 +6,7 @@
 whole_path="$(cd "$(dirname "$0")"; pwd)/$(basename "$0")" 
 srcname=$(basename "$whole_path")
 srcpath=$(dirname "$whole_path")
+projectpath="$(cd ../.. && pwd)" 
 [ ! -f ${srcpath}/appname ] && echo "FATAL: No appname to source" && exit
 [ -z "$appname" ] && . ${srcpath}/appname
 suspend="n" 
@@ -320,12 +321,14 @@
    setup_appcds $appcds $max_classes
 fi

+home="-DP2J_HOME=${projectpath}" 
+
 # Ensure UTF-8 encoding
 original_lang="${LANG}" 
 ensure_utf8_lang

 # run the FWD server
-JAVA_CMD="$prog $dump $perf $hprof $maxheap $srvr $jmx $dtxt $agent $spi $open_api $cpath $cdsparams $entrypoint $mode $batch $cfg $port $profile" 
+JAVA_CMD="$prog $dump $home $perf $hprof $maxheap $srvr $jmx $dtxt $agent $spi $open_api $cpath $cdsparams $entrypoint $mode $batch $cfg $port $profile" 
 if [ $test -eq 1 ] ; then
    echo "$JAVA_CMD" "$@" 
 elif [ $statcode = "y" ]; then

For the p2j:

=== modified file 'src/com/goldencode/p2j/util/SourceNameMapper.java'
--- old/src/com/goldencode/p2j/util/SourceNameMapper.java    2025-12-16 09:35:25 +0000
+++ new/src/com/goldencode/p2j/util/SourceNameMapper.java    2026-02-25 14:10:15 +0000
@@ -3465,6 +3465,7 @@
             URL resource = null;
             try
             {
+               nameMappingXmlPath = Configuration.getNameMapAsPath();
                resource = systemClassLoader.getResource(nameMappingXmlPath);
             }
             catch (Exception e)

Thank you for your interest and involvement to improve this.

Dănuț Filimon wrote:

How can I enable the developer mode?

Just create a new workspace in your IDE with your project (ex: hotel_gui), mark sources and libraries. Create run/debug configuration for conversion (ConversionDriver) / server (ServerDriver) and client (ClientDriver). Execute them directly from IDE, without intermediary packaging to jar.
Since the processes are started directly from IDE, the server.sh and client.sh are not used, the parameters go directly in run/debug configurations.

The actual problem is that the conversion and server processes are not started in same location. If the conversion output is <project-root>/cvt, and the server is started in <project-root>/deploy/server (it cannot be started anywhere else since that's the folder with runtime configuration: server.xml, directory.xml, etc), the default TAG_CVTPATH will make the server look for the resource in <project-root>/deploy/server/cvt when calling Configuration.getNameMapAsPath();. I think it is a bad option to store the configuration for these different processes in the same p2j.cfg.xml file. The relative paths will not match. I could use absolute path (like: /~/projects/fwd/7180/) but this means it needs updating if the project is moved or copied.

One solution here would be to allow the configuration parameters to be passed from command line, as P2J_HOME is at this moment.

Related to the change in SourceNameMapper. I think that is correct. The new nameMappingXmlPath points to valid resource (new File(nameMappingXmlPath).exists() returns true). The bad news is that systemClassLoader.getResource(nameMappingXmlPath) still returns null. I don't know why :(.

I will investigate that in the spare time and reply you back.

One solution here would be to allow the configuration parameters to be passed from command line

Yes, runtime configuration needs to be outside of p2j.cfg.xml. If it is needed at startup it must be on the command line or in the bootstrap cfg. Otherwise it should be in the directory.

Ovidiu Maxiniuc wrote:

Related to the change in SourceNameMapper. I think that is correct. The new nameMappingXmlPath points to valid resource (new File(nameMappingXmlPath).exists() returns true). The bad news is that systemClassLoader.getResource(nameMappingXmlPath) still returns null. I don't know why :(.

I will investigate that in the spare time and reply you back.

Can you add the cvt folder to the classpath and try again?

Yes, this works.

However, not by adding the classpath to run/debug configuration for the server process (the IDE will automatically add a second parameter and overwrite mine) but marking the <project-root>../../cvt as location for classes. Although there are no real classes to be loaded, the class-loader will use it for loading any resources. I do not know if this is good or not (probably it is), but this will expose the full set of conversion artefacts (*.schema, *.p2o, including *.cache) to be loaded by all processes I start as they would be in the working directory of the respective process. Of course, only if the resources are accessed via class-loader.

Note: in this case nameMappingXmlPath = Configuration.getNameMapAsPath(); is not needed. That is, the code is at r16438.

Returning to initial issue: the return at SourceNameMapper:3483 still makes the method exit, leaving some fields of this class uninitialzed. This is a kind of a limbo situation where mappingDataInitInProgess is true and mappingDataInitialized is false. The server is unusable, any attempt to connect a client will fail with the exceptions from note #7180-235.