FWD » Core Development » Base Language

We have applications which support 4GL code extensions that are not part of the core application itself. For example, customer/partner-written OO 4GL classes which extend application classes/implement application interfaces and which need to be loaded/accessed at runtime.

Although we do allow the propath to be assigned dynamically, we don't have any provision for the name_map.xml to be "sharded" (split across multiple files). Doing so would allow extension jars to be added on a per-installation basis and the name map entries for that extension jar would be loaded/added from the shard that resides in that extension jar.

Another improvement is to move to a load-time scanning approach where we build some (or all?) of the name mappings from annotations when the application code is loaded. If this does not generate too much of a performance hit to implement, then it is the preferred approach. In a perfect world, we would have no name_map.xml at all and there would be little cost in server startup time. If we go with this approach, then we should ensure that the extension jar case is handled as well.

Does each session need its own mappings based on what is in the propath? The application described above does not do that, but technically it would be possible in the 4GL.

6129a/13926 adds lazy loading of the legacy converted classes, to improve the FWD server startup time and also not load the entire .class for the legacy converted classes into the JVM, at once.

• at startup, SourceNameMapper loads the entire name_map.xml. For legacy converted classes, this is a registry of all converted classes (mapping of their Java name to legacy name), plus any virtual functions defined in the legacy class.
• for legacy converted classes, they structure is built when they are loaded by the application code (this is the 'lazy loading' approach).
• for external programs, although the LegacySignature and others are emitted in the converted code, we still rely on name_map.xml to build the entire structure, at server startup.

• refactor name_map.xml to use the same approach as for the legacy classes (emit only the virtual functions/internal procedures and other metadata which can't exist in the converted Java code)
• refactor SourceNameMapper to use a lazy-loading of the external program, and rely on the LegacySignature annotations
• another improvement would be to emit some kind of special constructs for the virtual functions/internal procedures directly in the converted code, and leave name_map.xml only as a registry for mapping web services and converted code to their Java counterpart.

We need to implement something soon due to some customer deadlines. I don't think we have time to implement the full annotations-based approach right now. Instead, I propose that we implement a refactored version of our loading process. The idea is we should support more than one name_map.xml found in the jars, with the loading process resulting in the same in-memory representation we have today, but just having been loaded from multiple files.

another improvement would be to emit some kind of special constructs for the virtual functions/internal procedures directly in the converted code, and leave name_map.xml only as a registry for mapping web services and converted code to their Java counterpart.

This is the right thing to do. Even for the web services, can't we include enough annotations so that we can build the "registry" by scanning? In a perfect world, there would be no name map at all and so long as our scanning process works across multiple jars, there is no issue with splitting conversion into many pieces.

Greg Shah wrote:

another improvement would be to emit some kind of special constructs for the virtual functions/internal procedures directly in the converted code, and leave name_map.xml only as a registry for mapping web services and converted code to their Java counterpart.

This is the right thing to do. Even for the web services, can't we include enough annotations so that we can build the "registry" by scanning? In a perfect world, there would be no name map at all and so long as our scanning process works across multiple jars, there is no issue with splitting conversion into many pieces.

We had this discussion before - moving everything to annotations will mean the entire converted .class code needs to be scanned and loaded into the JVM.

So, if we still don't want to load the entire app into the JVM at server startup, the goal is for name_map.xml be just a registry/mapping of converted program files/classes, and every other detail is in annotations.

We had this discussion before - moving everything to annotations will mean the entire converted .class code needs to be scanned and loaded into the JVM.

A good point. I don't remember this discussion but certainly we have to load the classes if we use runtime annotations. We are planning to pre-load a non-trivial percentage of the code via appcds, but we don't want to preload everything.

So, if we still don't want to load the entire app into the JVM at server startup, the goal is for name_map.xml be just a registry/mapping of converted program files/classes, and every other detail is in annotations.

I guess so.

MultiClassLoader has a list of all jars in the classpath. It can look for name_map.xml in each jar (with jar tvf filename.jar) and store the packages with name_map.xml in a list. Then use the packages in SourceNameMapper.initMappingData to load data in p2jMap, etc. Also search through them on each request for resolving something. The external uses of SourceNameMapper.getPackageRoot() will have to be reworked (ControlFlowOps, SoapHandler). If there are any clashes pkgroot will take precedence.

Probably some details will come up with implementation, but am I on the right track?

MultiClassLoader has a list of all jars in the classpath. It can look for name_map.xml in each jar (with jar tvf filename.jar) and store the packages with name_map.xml in a list.

We don't need to use a child process for this search. We can search in the jars directly. We have code in Utils for searching/reading resources out of jar files.

Probably some details will come up with implementation, but am I on the right track?

Yes

Galya, what package would you use for looking into multiple jars? Look everywhere? The problem with name_map.xml is that it can appear in multiple jars, under the same pkgroot. When app-by-app conversion is done (and after that we ran these in a single JVM), we don't want each sub-app to have its own pkgroot.

• look under pkgroot inside all jars
• load them into SourceNameMapper, one by one, and address conflicts - if there are conflicts, and the details in a name_map.xml are different than what was already loaded, log a warning.

Also, SourceNameMapper will not load the .class and annotations when the entry in name_map.xml is processed - this needs to be done lazily, when that converted program is first used.

Constantin Asofiei wrote:

Galya, what package would you use for looking into multiple jars? Look everywhere? The problem with name_map.xml is that it can appear in multiple jars, under the same pkgroot. When app-by-app conversion is done (and after that we ran these in a single JVM), we don't want each sub-app to have its own pkgroot.

Well, looking for one path in multiple jars is more problematic. Then I can't read the file from the classpath, but the jar needs to be unarchived and the xml file found on the file system and read.

That is of course if the check for the file listed by jar tvf filename.jar gives positive.

Why can't we use the same technique from Utils.searchResourceJars()? We don't need the propath part but we definitely don't want to use jar as a child process or unarchive things.

You don't need to unzip the jar - Java has tools to read an archive. This just means we can't rely on Class.getResource, and instead we need to check if jar if it has a zip-entry with this exact path and name (pkgroot/name_map.xml)

Constantin Asofiei wrote:

Java has tools to read an archive.

If you mean the custom FWD's JarClassLoader, then it should work.

Constantin Asofiei wrote:

Also, SourceNameMapper will not load the .class and annotations when the entry in name_map.xml is processed - this needs to be done lazily, when that converted program is first used.

Which is 'that converted program'? We don't know what's inside the name map before it's read. Since all name maps will be in the same package, they need to be read at the same time, otherwise you won't know for sure what the classloader has loaded.

SourceNameMapper.initMappingData seems to be executed only once for the lifespan of the runtime machine with a server start hook, so that's where the checks will be performed over all the maps at the same time. Am I missing something?

Galya B wrote:

SourceNameMapper.initMappingData seems to be executed only once for the lifespan of the runtime machine with a server start hook, so that's where the checks will be performed over all the maps at the same time. Am I missing something?

Correct.

Which is 'that converted program'? We don't know what's inside the name map before it's read. Since all name maps will be in the same package, they need to be read at the same time, otherwise you won't know for sure what the classloader has loaded.

I mean we don't load Class.forName (to look at annotations) until that converted program is targeted by a RUN statement.

Constantin Asofiei wrote:

• load them into SourceNameMapper, one by one, and address conflicts - if there are conflicts, and the details in a name_map.xml are different than what was already loaded, log a warning.

I'm not sure what the criteria for equality is. For example servicePrograms are of type ExternalProgram and have a lot of properties. Do you expect the comparison to go to the lowest level or just log a warning on attempt to add a second service program with the same pname that may or may not be exactly the same?

I don't mean the annotations in the .class - I mean the state from name_map.xml must match if there is an existing entry from a previous name_map.xml

In the end, name_map.xml must end up only with a very short registry of just legacy name to Java names.

Constantin Asofiei wrote:

I don't mean the annotations in the .class - I mean the state from name_map.xml must match if there is an existing entry from a previous name_map.xml

servicePrograms gets populated by reading name_map.xml. What annotations? buildExternalProgram is done during reading the map and populates pname, jname, ooname, publishedEvents, ieMap, p2jf, j2pf, main that is InternalEntry with its own parameters populated.

So all of this should be matching?

The point was to move to annotations/other structures inside the .java, here I mean all info from name_map.xml; ideally, we would be left just with:

   <class-mapping jname="Test" pname="test.p"/>

Until this is done, the entire class-mapping node must match - the key to match should be the same Java converted class name.

Constantin Asofiei wrote:

The point was to move to annotations/other structures inside the .java

Constantin, maybe you've missed why I'm assigned to the task, that is in #6407-4.

Greg Shah wrote:

We need to implement something soon due to some customer deadlines. I don't think we have time to implement the full annotations-based approach right now. Instead, I propose that we implement a refactored version of our loading process. The idea is we should support more than one name_map.xml found in the jars, with the loading process resulting in the same in-memory representation we have today, but just having been loaded from multiple files.

Galya B wrote:

Constantin, maybe you've missed why I'm assigned to the task, that is in #6407-4.

OK, thanks for the remainder. Then the entire class-mapping node needs to match if it exists in multiple name_map.xml.

Constantin Asofiei wrote:

Galya B wrote:

Constantin, maybe you've missed why I'm assigned to the task, that is in #6407-4.

OK, thanks for the remainder. Then the entire class-mapping node needs to match if it exists in multiple name_map.xml.

And all of this just for a warning log? I would say it's more reasonable to log the warning always when a second entry with the same pname is found.

Galya B wrote:

And all of this just for a warning log? I would say it's more reasonable to log the warning always when a second entry with the same pname is found.

If the entries are different, then there is something very bad with the conversion. This is not a warning, is a fatal error.

Constantin Asofiei wrote:

Galya B wrote:

And all of this just for a warning log? I would say it's more reasonable to log the warning always when a second entry with the same pname is found.

If the entries are different, then there is something very bad with the conversion. This is not a warning, is a fatal error.

So the server should exit in this case?

Yes.

OK, then I need one more clarification. Since there is a separate container rest-service, where programs get resolved by adding postfix -fwd-rest-service.p to pnames, do we consider these different from the regular class-mapping with the same pname or if the pname is the same without the postfix this is a clash?

REST services written directly in Java can be configured via the 'rest-service' element.

So these nodes are not from conversion, but added via name_map_merge.xml - they can be ignored for this match.

And all of this just for a warning log? I would say it's more reasonable to log the warning always when a second entry with the same pname is found.

If the entries are different, then there is something very bad with the conversion. This is not a warning, is a fatal error.

Won't this happen anytime there is an overlap of the same relative program names in 2 different apps?

Greg Shah wrote:

And all of this just for a warning log? I would say it's more reasonable to log the warning always when a second entry with the same pname is found.

If the entries are different, then there is something very bad with the conversion. This is not a warning, is a fatal error.

Won't this happen anytime there is an overlap of the same relative program names in 2 different apps?

If the class info is the same, there will be no clash, no warning and exit.

I don't have an actual example though. Not sure if the path is supposed to be the same in two different jars.

Greg Shah wrote:

And all of this just for a warning log? I would say it's more reasonable to log the warning always when a second entry with the same pname is found.

If the entries are different, then there is something very bad with the conversion. This is not a warning, is a fatal error.

Won't this happen anytime there is an overlap of the same relative program names in 2 different apps?

The 'pname' in class-mapping is the full path relative to the abl/ folder - we don't use relative legacy names in name_map.xml, they are all 'full' relative to abl/.

• if the same .p/.cls (as full name relative to abl/) is converted in multiple modules, then is assumed that is the same physical file in all modules. I don't know how we can enforce this.
• all modules need to convert with the same root package name, as 'jname' is relative to the root Java package.

The 'pname' in class-mapping is the full path relative to the abl/ folder

Yes, this is why I called them relative paths.

if the same .p/.cls (as full name relative to abl/) is converted in multiple modules, then is assumed that is the same physical file in all modules. I don't know how we can enforce this.

Let's consider this from the perspective of how it works for this multi-app case in OE. If both are compiled separately and then included in the same runtime installation, each .r would actually be in a different path right? Thus the runtime propath would be able to differentiate them. I suspect we need to make these unique by adding a per-app directory.

all modules need to convert with the same root package name, as 'jname' is relative to the root Java package.

I wonder if this is right. Similar to the pname case, perhaps we should be having a unique root package (app-level sub-packages)?

Originally (currently) if multiple paths are found for the same filename when resolving the name map, the paths end up in MultiPathLookup, that returns null for clashes on lookup:

      public String lookupPath(String legacyProgName, String[] propath, boolean caseSens)
      {
         String result = null;
         for (SinglePathLookup spl : references)
         {
            String possibleResult = spl.lookupPath(legacyProgName, propath, caseSens);
            if (result == null && possibleResult != null)
            {
               result = possibleResult;
               continue;
            }

            // The result is already set, so any possible result is a conflict and should return null
            // in order to fallback to the old implementation
            if (possibleResult != null)
            {
               return null;
            }
         }
         return result;
      }

Greg Shah wrote:

Let's consider this from the perspective of how it works for this multi-app case in OE. If both are compiled separately and then included in the same runtime installation, each .r would actually be in a different path right? Thus the runtime propath would be able to differentiate them. I suspect we need to make these unique by adding a per-app directory.

I don't understand. Are you talking about duck-type-ing? If a .r for file F is needed by two modules, and is compiled in module M1 and module M2, then F.r must be in the same path, even if is included in more than one .pl.

If you are saying that module M1 has a program foo/bar/F.p and module M2 has a program foo/bar/F.p. which are completely different programs and unrelated to each other, then these need to be differentiated via specific folders in abl/. And the PROPATH (static or at runtime) is responsible for making sure the correct one is picked up.

all modules need to convert with the same root package name, as 'jname' is relative to the root Java package.

I wonder if this is right. Similar to the pname case, perhaps we should be having a unique root package (app-level sub-packages)?

What about the API (interfaces) which are common for 2 or more modules, which are converted in each module? We can't change the Java package for these.

This is actually more complicated than it looked at first. We're trying to combine different apps coming possibly from different propaths in OE and having diverging files included into one FWD instance. In java this is the same as having multiple versions of the same dependency in the classpath. Constantin seems to be against the idea of supporting this and I can understand why, thinking of all the issues we have with this in java. But on the other hand how can we make sure the customer is not compiling different apps with different versions of the same file.

If you are saying that module M1 has a program foo/bar/F.p and module M2 has a program foo/bar/F.p. which are completely different programs and unrelated to each other, then these need to be differentiated via specific folders in abl/. And the PROPATH (static or at runtime) is responsible for making sure the correct one is picked up.

Exactly. But in OE, they don't have to do that if they install the compiled results in different directories. So: if in OE, the separate modules are installed in different directories, then we need to add those directories at conversion time. If we add that requirement, it will work in FWD.

On the other hand, it might be that the customer builds multiple apps separately in OE but then installs them all in the same location. In such a case, the conflicting .r files would overwrite each other, with the last one installed being the one that "wins". In that case, we can implement a precedence approach as well, with the last one loaded winning. As long as the loading order in FWD is the same as the installation order in OE, it will work the same way.

Either way, we can (and should) resolve this without abending the server.

What about the API (interfaces) which are common for 2 or more modules, which are converted in each module? We can't change the Java package for these.

A good point. Especially because of the OO4GL stuff but even for procedures, any truly common code must have the same paths in any app being converted. This makes it more important to ensure that we have a clean way to handle conflicts as noted above.

In java this is the same as having multiple versions of the same dependency in the classpath.

Not exactly. In Java, the same exact class name can't be loaded twice in the same JVM. That is different from OE. The Java classnames are fully qualified and only one will win.

There are 2 cases in OE. Case 1 is the "installed in different locations" and that is perfectly fine in OE. You can in fact run both versions if you setup your propath differently for each RUN statement. Case 2 is the "overwriting compiled results" case and in that case, there is only 1 version of that program at runtime so it wins.

Constantin seems to be against the idea of supporting this and I can understand why, thinking of all the issues we have with this in java. But on the other hand how can we make sure the customer is not compiling different apps with different versions of the same file.

I think we can resolve both cases in a manner that is roughly equivalent to the OE result.

Case 2 is the same as java and can cause malfunctioning programs like in java, this is what I meant.
Case 1 is fine in FWD too, the only requirement is to have in the jar the same directory for name_map.xml as declared in pkgroot. Other than that the jar can have a completely different package for the actual classes.

If we need to load the jars in a specific order, we'll need explicit instructions, probably new config in directory.

Galya B wrote:

Other than that the jar can have a completely different package for the actual classes.

Actually not quite, since we support only one pkgroot, but anyways the new app can be nested, it's not relevant to where the name_map.xml is.

One pname corresponds to only one jname, because that's how conversion seems to work. The question is if the other attributes of the mapping are the same or different between the two places where the mappings are configured. In any case only one java class will be loaded in the runtime classpath, so only one of those configs will be valid. This is not something we can decide even with configs and Constantin was right that it is a major issue.

As of how the same file is loaded from different paths, I think this is already handled by the lookup in MultiPathLookup.

This is not something we can decide even with configs and Constantin was right that it is a major issue.

Case 1 can be handled at conversion time by adding an app-specific path segment to make the filenames unique. There is no change needed for runtime support.

Case 2 can be handled by implementing a precedence order to honor the first loaded or the last loaded.

I speak about case 3: different maps for different files with different attributes, but same path. Obviously if you load the wrong file it won't match the map.

Galya B wrote:

I speak about case 3: different maps for different files with different attributes. Obviously if you load the wrong file it won't match the map.

and one of the two apps won't work, even if we implement precedence and load one

To solve case 2. we need the precedence order to be fed to the jmv via startup args, so that the classloader can take it in consideration in findClass and loadClass.

To solve case 3. probably best to abend or one of the apps will malfunction.

We had cases of the same OE qualified class name used in different modules. The qualified legacy name can't be changed by our conversion, no matter if we place it in a different Java package. The solution for this was for the customer to change the legacy 4GL code, so that there are no collisions in qualified legacy class names.

To be more specific, the collision was for a .cls in module M3 and M2, which both got bundled separately as (M1, M3) and (M2, M3) when deployed in OpenEdge, but in FWD we bundled them all as (M1, M2, M3).

• we have M1 dependent on I2 (API interfaces) for module M2
• in OpenEdge, M1 and I2 are used for a build (not deploy) and (M2, I2) for the other build.
• if a customer requires M1, the .pl for M1 just gets released alongside the .pl for (M2, I2) (the .pl is like a .jar in Java)
• in FWD, we want to convert (M1, I2) and (M2, I2) separately
• however we place the I2 classes in the abl/ folder, this must be the same for both cases, the conversion for (M1, I2) and the conversion for (M2, I2) must produce the same Java qualified class names for the code in I2.
• I don't know if this works or not, but it's worth noting: we could convert first just I2, add this jar to the conversion classpath, and FWD should be able to convert just M1 without the I2 legacy code. But I don't know if this would work or not (haven't tested).
• the conclusion: when legacy code for inter-module dependencies are converted via multiple modules, the generated Java qualified class name (and its actual code actually) must match
  • for .cls is obvious, we break the Java code if we use different Java packages for the same interface in different modules
  • for .p is a little more subtle: you will end up with duplicated Java code if the same .p is converted in module M1 under abl/module1/foo/bar.p and under M2 under abl/module2/foo/bar.p

So, my problem is not what package root we use for the converted code which is not shared between modules - we use this paradigm today, we create different abl/ folders sub-folders and place the .cls and .p under that. My problem is if we want to automate this, what happens (and how we distinguish) that some .cls/.p is shared code or not.

Also, about the .r code - I agree with Greg that 'is possible' for a deploy which just uses .r code and copies the files (in a sequence) to some folder (overwriting files maybe), and the result is the application. But, in practice, I think .pl archives are used more - and depending on the PROPATH, runtime could use a .p from different .pl files, even if the full path matches.

But, in practice, I think .pl archives are used more - and depending on the PROPATH, runtime could use a .p from different .pl files, even if the full path matches.

Yes, the .pl scenario is "case 1" and "just works" in OE.

Greg Shah wrote:

But, in practice, I think .pl archives are used more - and depending on the PROPATH, runtime could use a .p from different .pl files, even if the full path matches.

Yes, the .pl scenario is "case 1" and "just works" in OE.

And in FWD the abl/ sub-folders where we place each module is the equivalent of the .pl 'namespace'.

• in FWD, you would convert the module code without abl/ sub-folders, and whatever mapping in name_map.xml is first for that pname (or maybe some rules), that would be 'it'; a warning can be logged for this. But, even if we have different Java packages for the sub-modules, the converted .class will still exist for all the pnames which match. And if we don't different Java packages for sub-modules, then is a matter of the Java classpath to place the 'proper' jar in the correct position in the classpath, otherwise JVM will load the wrong .class.
• but, my original concern is for the case when shared code is being converted in multiple modules; in this case, if there is a mismatch, then that is bad, and a warning I don't think is enough. You could end up with the same Java qualified class name for two different legacy classes.

For classes, if the mapping of qualified Java name and legacy name are the same, then considering that we can't check all the annotations in the class (or its actual content), a warning I think it should be logged, also (maybe on a lower level like FINE).

For external programs, on a second thought, we will end up with all internal entries configurations at annotations/in the converted .class, so I think we can keep the same, look at only the class-mapping attributes.

Constantin Asofiei wrote:

And I think I understand now Greg's point about the .r and folder deploy in OpenEdge:

• in FWD, you would convert the module code without abl/ sub-folders, and whatever mapping in name_map.xml is first for that pname (or maybe some rules), that would be 'it'; a warning can be logged for this. But, even if we have different Java packages for the sub-modules, the converted .class will still exist for all the pnames which match. And if we don't different Java packages for sub-modules, then is a matter of the Java classpath to place the 'proper' jar in the correct position in the classpath, otherwise JVM will load the wrong .class.

case 2: the precedence order to be fed to the jmv via startup args, so that the classloader can take it in consideration in findClass and loadClass.

• but, my original concern is for the case when shared code is being converted in multiple modules; in this case, if there is a mismatch, then that is bad, and a warning I don't think is enough. You could end up with the same Java qualified class name for two different legacy classes.

case 3: probably best to abend or one of the apps will malfunction.

For classes, if the mapping of qualified Java name and legacy name are the same, then considering that we can't check all the annotations in the class (or its actual content), a warning I think it should be logged, also (maybe on a lower level like FINE).

For external programs, on a second thought, we will end up with all internal entries configurations at annotations/in the converted .class, so I think we can keep the same, look at only the class-mapping attributes.

One java name can have only one mapping. And legacy names are resolved by the java name. So a critical mismatch leading to server exit is the same jname, but different ooname or pname. A duplicate entry is the same jname with the same ooname and pname and results in a FINE log and we expect the issue to be solved by the jar precedence in the classloader. Is this right?

P.S. The previous comment got updated.

From what you posted in #6407-56, only one concern: the granularity for the precedence order I think needs to be "this .class from this .jar first", not "this .jar first".

Constantin Asofiei wrote:

From what you posted in #6407-56, only one concern: the granularity for the precedence order I think needs to be "this .class from this .jar first", not "this .jar first".

Something like this would be very verbose if a clash in a whole module needs to be resolved: -Dclassloader.order=com.example.Class1:first.jar,second.jar;com.example.Class2:first.jar,second.jar.

Do we really need an explicit list of precedence order? Why not implicitly take it from the classpath and honor the first found?

Greg Shah wrote:

Do we really need an explicit list of precedence order? Why not implicitly take it from the classpath and honor the first found?

The classpath has the jars loaded randomly. What is there to do then? Just end up with whatever.

Galya B wrote:

Greg Shah wrote:

Do we really need an explicit list of precedence order? Why not implicitly take it from the classpath and honor the first found?

The classpath has the jars loaded randomly. What is there to do then? Just end up with whatever.

My understanding is that JVM keeps the same order of jars as defined in classpath (it does not use a random order). I've been adding bin/ to the start of propath before p2j.jar since forever for my local testing and it always worked.

If our classloaders are doing something different, then that may be a problem at some point.

Constantin Asofiei wrote:

My understanding is that JVM keeps the same order of jars as defined in classpath (it does not use a random order). I've been adding bin/ to the start of propath before p2j.jar since forever for my local testing and it always worked.

If our classloaders are doing something different, then that may be a problem at some point.

MultiClassLoader finds the class by iterating HashSet. You can't rely on a specific order of iteration, even less to be similar to the java implementation. I wouldn't bet even on all the clashing classes to be loaded from the same jar.

We can rework it to iterate jars alphabetically.

Galya B wrote:

MultiClassLoader finds the class by iterating HashSet.

Actually it's a collection, backed by the HashMap, so the order is still decided by the internal order of a HashMap. So what I'm saying is still valid.

MultiClassLoader finds the class by iterating HashSet.

Actually it's a collection, backed by the HashMap, so the order is still decided by the internal order of a HashMap. So what I'm saying is still valid.

If we only add one match to the collection for each fully qualified class, then there is no issue. We just have to ensure we add in the classpath order.

Greg Shah wrote:

MultiClassLoader finds the class by iterating HashSet.

Actually it's a collection, backed by the HashMap, so the order is still decided by the internal order of a HashMap. So what I'm saying is still valid.

If we only add one match to the collection for each fully qualified class, then there is no issue. We just have to ensure we add in the classpath order.

It's a HashMap of jars, the loader iterates over the values, that are jar classloaders. The first jar having the class wins. So it's random.

So what I'm saying is that there is no classpath order in my humble opinion. Iterating HashMap is not ordered.

MultiClassLoader:

   /** Map containing as key the jar name and as value its associated class loader. */
   private final Map<String, JarClassLoader> loaders = new HashMap<>();

   /** A set containing all jars added to the classpath when the server was started. */
   private static final Set<String> classpathJars = JarUtil.resolveClasspathJars(null);

I did replace the Set with List in r15108.

• running the original code
• the new branch - the map is split in two and a new jar added. Then try to run a few different procs from both maps and evaluate their success.

Reminder: The branch needs a review (at r15110). It also has minor changes for #8166-145 and #8166-150.

6407a rebased on trunk r15146.
New r15156 with a minor fix for #8166-151.

• WebServer - please add a history entry
• Utils.packageToPath - this replaces all dots with File.separatorChar - on Windows, this is '\'. Is this valid when working with jar resources? My understanding is that jar resources are always using '/' linux-style separator.
• SourceNameMapper - convertJavaProg is used during conversion via NameMappingWorker.classNameExists, to check for collisions. This requires to read the name_map.xml file from the file system, and not jar. This needs to be fixed.

Otherwise, we need to do some conversion testing with these changes. And some small runtime tests.

Constantin Asofiei wrote:

Review for 6407a rev 15157:

• WebServer - please add a history entry

Fixed.

• Utils.packageToPath - this replaces all dots with File.separatorChar - on Windows, this is '\'. Is this valid when working with jar resources? My understanding is that jar resources are always using '/' linux-style separator.

The reason I had to make Utils.packageToPath to be OS specific is JarClassLoader.containsResource check in SourceNameMapper.initMappingData that compares the concatenated path for NAME_MAP_FILE with the resource paths in JarClassLoader.resources coming from the classpath that has OS specific file dividers.

Now that I look closer, I actually need to change how the first char is stripped off the resource name in JarClassLoader.containsResource and getResourceName to work for Windows.

• SourceNameMapper - convertJavaProg is used during conversion via NameMappingWorker.classNameExists, to check for collisions. This requires to read the name_map.xml file from the file system, and not jar. This needs to be fixed.

I'm not sure what the change is here?

Otherwise, we need to do some conversion testing with these changes. And some small runtime tests.

I did manually conversions of a few procedures and it works. What do we look for? I'm not sure what impact is expected on conversion.

Galya B wrote:

• SourceNameMapper - convertJavaProg is used during conversion via NameMappingWorker.classNameExists, to check for collisions. This requires to read the name_map.xml file from the file system, and not jar. This needs to be fixed.

I'm not sure what the change is here?

If there is a name_map.xml on the file system, use that, too. My understanding from the code is that only jars are used for name_map.xml

I did manually conversions of a few procedures and it works. What do we look for? I'm not sure what impact is expected on conversion.

We need to test full conversion of some customer projects.

Constantin Asofiei wrote:

Galya B wrote:

• SourceNameMapper - convertJavaProg is used during conversion via NameMappingWorker.classNameExists, to check for collisions. This requires to read the name_map.xml file from the file system, and not jar. This needs to be fixed.

I'm not sure what the change is here?

If there is a name_map.xml on the file system, use that, too. My understanding from the code is that only jars are used for name_map.xml

Where is that? Not in the original initMappingData as far as I can see. NameMappingWorker provides only two args pkgroot and name that is The class name we want to check for existence.. initMappingData only works with one path, that is URL url = ClassLoader.getSystemResource(sb.toString());, so the resource is always read from the ClassLoader and that is the classpath, i.e. the jars.

You are correct, at this point, unless the current folder is in the classpath, ClassLoader.getSystemResource(sb.toString()); will not find a name_map.xml from file-system. The goal of classNameExists was to ensure that we can emit unqualified FWD classes in conversion safely (this is used just for Action at this point); I think it may have been broken from the beginning, testing just from Eclipse where bin/ is in the classpath may have been the 'wrong indication' that it was working.

With the classNameExists in mind, these changes don't affect conversion.

Constantin Asofiei wrote:

You are correct, at this point, unless the current folder is in the classpath, ClassLoader.getSystemResource(sb.toString()); will not find a name_map.xml from file-system. The goal of classNameExists was to ensure that we can emit unqualified FWD classes in conversion safely (this is used just for Action at this point); I think it may have been broken from the beginning, testing just from Eclipse where bin/ is in the classpath may have been the 'wrong indication' that it was working.

Actually if a dir with name map file is in the classpath the name map should have been found in the original code in the dir under the custom package root (NameMappingWorker.pkgroot defaults to empty if not set in Configuration ), while currently it only iterates the jars. If such scenario exists, then I can add back a simple ClassLoader.getSystemResource, but only in case of no name maps found in jars, otherwise it will get complicated. How does it sound?

Galya B wrote:

Actually if a dir with name map file is in the classpath the name map should have been found in the original code in the dir under the custom package root (NameMappingWorker.pkgroot defaults to empty if not set in Configuration ), while currently it only iterates the jars. If such scenario exists, then I can add back a simple ClassLoader.getSystemResource, but only in case of no name maps found in jars, otherwise it will get complicated. How does it sound?

Yes, but I don't think we ever set . or other dir in the classpath, in the build scripts we have for conversion.

Try two programs named action.p and action2.p with just this content:

block-level on error undo, throw.

message "x".

I suspect with or without 6407a, this will not emit BlockManager.Action.THROW, but only Action.THROW.

Constantin Asofiei wrote:

Galya B wrote:

Actually if a dir with name map file is in the classpath the name map should have been found in the original code in the dir under the custom package root (NameMappingWorker.pkgroot defaults to empty if not set in Configuration ), while currently it only iterates the jars. If such scenario exists, then I can add back a simple ClassLoader.getSystemResource, but only in case of no name maps found in jars, otherwise it will get complicated. How does it sound?

Yes, but I don't think we ever set . or other dir in the classpath, in the build scripts we have for conversion.

Try two programs named action.p and action2.p with just this content:
[...]

I suspect with or without 6407a, this will not emit BlockManager.Action.THROW, but only Action.THROW.

With or without 6407a I get:

Action.java:25: error: cannot find symbol
    [javac]          onBlockLevel(Condition.ERROR, Action.THROW);

Not sure what it means.

Is there something else I need to do for this task?

Galya B wrote:

With or without 6407a I get:
[...]

Not sure what it means.

There is ambiguity between the Action.java for converted code and BlockManager.Action unqualified inner class name.

Is there something else I need to do for this task?

I don't think so.

Regarding the Utils.packageToPath change - please check runtime on Windows for i.e. Hotel GUI.

Constantin Asofiei wrote:

Regarding the Utils.packageToPath change - please check runtime on Windows for i.e. Hotel GUI.

I can't test hotel_gui (or any web ui) in a VM, because the web server gets the url from the ip of the network interface, that is something 10.xx.xx.xx and then the certificate doesn't match and the browser gives ERR_SSL_PROTOCOL_ERROR that can't be avoided.
I've tested to convert one procedure with testcases and it works. Not sure what else to test in Win.

Galya B wrote:

Not sure what else to test in Win.

Try the Swing client for Hotel GUI on Windows.

There is an issue with the file separators indeed. It's \ on Windows for the jars on the file system, but the resources in the jars always use Linux style separators. Fixed in r15159.

I think this is the only issue related to the task. I couldn't start the hotel_gui swing client because there is something going on with ant import.db, but it's not related:

C:\code\hotel_gui\build_db.xml:42: java.io.IOException: Cannot run program "C:\JavaCoretto\jdk1.8.0_402\jre\bin\java.exe" (in directory "C:\code\hotel_gui"): CreateProcess error=206, The filename or extension is too long
        at java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)

And it's not the java path. Not sure what is misconfigured. I haven't run it before on Windows.

OK, the java command for the task create.db.h2 exceeds 8191 chars (the max length of commands in windows) due to the classpath:

   <!-- path used when running application related tasks-->
   <path id="app.classpath">
      <fileset dir="${fwd.lib.home}"    includes="*.jar"/>
      <fileset dir="${deploy.home}/lib" includes="*.jar"/>
   </path>

When one of the two dirs is removed, the task gets through.

I don't know what's going on now, but the client doesn't even spawn and I can't stop it in the cmd prompt. No errors in logs. jstack suggests a warning msg is to be displayed, but no signs of any window. I think the warning should be the one I've just described in #8689:

"main" #1 prio=5 os_prio=0 tid=0x000002cc5f647800 nid=0xf54 in Object.wait() [0x000000ec93bfe000]
   java.lang.Thread.State: TIMED_WAITING (on object monitor)
    at java.lang.Object.wait(Native Method)
    at com.goldencode.p2j.ui.client.TypeAhead.getKeystroke(TypeAhead.java:447)
    - locked <0x0000000714c3a088> (a com.goldencode.p2j.ui.client.TypeAhead)
    at com.goldencode.p2j.ui.chui.ThinClient.modalEventLoopWorker(ThinClient.java:10324)
    at com.goldencode.p2j.ui.chui.ThinClient.modalEventLoop(ThinClient.java:10199)
    at com.goldencode.p2j.ui.chui.ThinClient.lambda$messageBox$36(ThinClient.java:10136)
    at com.goldencode.p2j.ui.chui.ThinClient$$Lambda$386/1544518128.run(Unknown Source)
    at com.goldencode.p2j.ui.chui.ThinClient.withIndependentEventList(ThinClient.java:10250)
    at com.goldencode.p2j.ui.chui.ThinClient.messageBox(ThinClient.java:10106)
    at com.goldencode.p2j.ui.chui.ThinClient.messageBox(ThinClient.java:9951)
    at com.goldencode.p2j.ui.chui.ThinClient.messageBox(ThinClient.java:9882)
    at com.goldencode.p2j.ui.chui.ThinClient.displayWarningMessage(ThinClient.java:9811)
    at com.goldencode.p2j.ui.ErrorWriterInteractive.displayWarning(ErrorWriterInteractive.java:138)
    at com.goldencode.p2j.util.ErrorManager.displayWarning(ErrorManager.java:3079)
    at com.goldencode.p2j.ui.chui.ThinClient.displayWarning(ThinClient.java:9575)
    at com.goldencode.p2j.ui.ClientExportsMethodAccess.invoke(Unknown Source)
    at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:156)
    at com.goldencode.p2j.util.TraceHelper.trace(TraceHelper.java:145)
    at com.goldencode.p2j.net.Dispatcher.trace(Dispatcher.java:1083)
    at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:787)
    at com.goldencode.p2j.net.Conversation.block(Conversation.java:422)
    at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
    at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1221)
    at com.goldencode.p2j.net.Queue.transact(Queue.java:682)
    at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:273)
    at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:221)
    at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1466)
    at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:144)
    at com.sun.proxy.$Proxy3.standardEntry(Unknown Source)
    at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:495)
    at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:284)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:593)
    at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:378)
    at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:425)

Constantin, do I need to make hotel_gui or testcases run on Windows as part of this task, because this is far off as of now?

Greg Shah wrote:

Constantin: Are we good to go?

My main concern with Windows was that jar file separator which needs to be linux-style, and this was solved. Otherwise, if some testing with customers app works, I'm OK with the changes.

6407a rebased on trunk r15171.

Constantin Asofiei wrote:

Otherwise, if some testing with customers app works, I'm OK with the changes.

I've run the code with the m project and the app loads fine. I'm not sure what else to look for.

Constantin: Are there any reasons why this can't be merged to trunk?

Greg Shah wrote:

Constantin: Are there any reasons why this can't be merged to trunk?

No, it can be merged.

In #6667-995, Tomasz found that the server failed to start after picking up trunk rev 15180. The issue turned out to be the same application jar specified multiple times in the classpath.

This means we are newly sensitive to this kind of misconfiguration. I suspect this will get worse when customers start implementing full app by app conversion. They will have to be very careful to avoid including common classes into multiple application jars. That may be quite a mess.

I thought we made this work without being fatal. Didn't we decide to allow the first class of a given fully qualified name to be the one that was "honored"?
"

Greg Shah wrote:

In #6667-995, Tomasz found that the server failed to start after picking up trunk rev 15180. The issue turned out to be the same application jar specified multiple times in the classpath.

This means we are newly sensitive to this kind of misconfiguration. I suspect this will get worse when customers start implementing full app by app conversion. They will have to be very careful to avoid including common classes into multiple application jars. That may be quite a mess.

I thought we made this work without being fatal. Didn't we decide to allow the first class of a given fully qualified name to be the one that was "honored"?

Interesting. Most of the discussion in this task is about how map resolvement should work and it's implemented according to our decisions. I've clearly explained that there is no strict order in HashSet of MultiClassLoader and you didn't want explicit precedence as java argument. So any conflicts of content in both maps should result in a fatal outcome as explained in #6407-55, where Constantin says:

if there is a mismatch, then that is bad, and a warning I don't think is enough. You could end up with the same Java qualified class name for two different legacy classes.

For classes, if the mapping of qualified Java name and legacy name are the same, then considering that we can't check all the annotations in the class (or its actual content), a warning I think it should be logged, also (maybe on a lower level like FINE).

For external programs, on a second thought, we will end up with all internal entries configurations at annotations/in the converted .class, so I think we can keep the same, look at only the class-mapping attributes.

I don't think the issue in #6667-995 is the same jar path twice in the classpath, but obviously two similar jars in different paths. The first time Roger added the build/lib jars explicitly to the classpath. The second time his script added ../lib/*.jar to the classpath (that is obviously in the deploy dir).

The second jar with the same java class name triggers this check:

ExternalProgram resolvedProgram = j2pMap.get(jname);
if (!resolvedProgram.pname.equals(pname) || !ooname.equals(resolvedProgram.ooname))
{
   LOG.severe("Mapping for class " + jname + " found multiple times with different attributes.");
   System.exit(-1);
}

How do you define precedence and what is the criteria of stacking jars?

I think we should actually be happy to have found this discrepancy in the scripts. Every time such malfunction appears, this is a successful test (that should have been found in Jenkins).

And let me state again, if there is a discrepancy in the content of the maps of two jars in the classpath, this is a serious issue that should not be swept under the carpet, as Constantin has explained earlier. Otherwise there will be runtime issues.

Interesting. Most of the discussion in this task is about how map resolvement should work and it's implemented according to our decisions.

I thought that from #6407-56 (and later), that we would mimic the same approach as Java ("the first one found in the classpath wins").

I've clearly explained that there is no strict order in HashSet of MultiClassLoader and you didn't want explicit precedence as java argument.

I thought that in #6407-71, this was changed to a List approach which has proper FIFO ordering. Or we could use a FIFO Set.

So any conflicts of content in both maps should result in a fatal outcome as explained in #6407-55, where Constantin says:

This does not have to be the case if we just implement precedence. No need for a configuration option, since we can just follow the standard Java approach. For the app by app scenario, it may be quite time consuming to get it right.

I agree that in the case where there are different instances of the same class, it is a real configuration problem that is hard to debug. But in the case of the same class appearing multiple times, it is just a new problem to solve for the customer. Perhaps we should implement a configurable option (failOnDuplicateClass). By default we leave it like in Java. If the option is enabled, we make it a fatal error. But if we do make it fatal, I think we must calculate the full list of overlaps before exiting otherwise it will have to be done by the customer who doesn't have tooling to help.

I don't think the issue in #6667-995 is the same jar path twice in the classpath, but obviously two similar jars in different paths. The first time Roger added the build/lib jars explicitly to the classpath. The second time his script added ../lib/*.jar to the classpath (that is obviously in the deploy dir).

Why do you think they are different? In fact, it is more likely that they are the same. The version in build/lib is commonly copied to deploy/lib/ using ant deploy.prepare.

The second jar with the same java class name triggers this check:

[...]

How do you define precedence and what is the criteria of stacking jars?

First one in the classpath wins, just as in Java.

As far as the log message, we have no information to know that "with different attributes" is correct. Putting that in the message is confusing. It suggests we are doing a deeper look at the classes/content than is happening.

Greg, why would #6667-995 fail with this test ((!resolvedProgram.pname.equals(pname) || !ooname.equals(resolvedProgram.ooname))), if the same, physical, application .jar was used twice in the classpath? The failure happens only and only if the legacy program name or legacy OE class name do not match, for a converted Java class name.

Tomasz: is it possible that an old and a new jar was in the classpath?

Greg, why would #6667-995 fail with this test ((!resolvedProgram.pname.equals(pname) || !ooname.equals(resolvedProgram.ooname))), if the same, physical, application .jar was used twice in the classpath? The failure happens only and only if the legacy program name or legacy OE class name do not match, for a converted Java class name.

Good point.

The comparison in the condition is solid... if it wasn't for extProg.ooname = ooname.isEmpty() ? null : ooname;. ooname gets read as empty string, but gets saved as null.

org.w3c.dom.Element:

     * @return The Attr value as a string, or the empty string
     *   if that attribute does not have a specified or default value.
     */
    public String getAttribute(String name);

So the necessary change is:

            if (!resolvedProgram.pname.equals(pname) || 
               (!(ooname.isEmpty() && resolvedProgram.ooname == null) && !ooname.equals(resolvedProgram.ooname)))
            {
               LOG.severe("Mapping for class " + jname + " found multiple times with different attributes.");
               System.exit(-1);
            }

Do I create a branch for it or add it to 8667a that will potentially be merged soon?

You can include it in 8667a.

Greg Shah wrote:

You can include it in 8667a.

The fix was merged to trunk as rev. 15216.

Greg Shah wrote:

I thought that in #6407-71, this was changed to a List approach which has proper FIFO ordering. Or we could use a FIFO Set.

Not initiating a discussion, because the issue is solved and the code is working properly. Just explaining what happened with #6407-71: I got it reverted before the review, because I didn't see much enthusiasm for the idea and also found eventually that it's not relevant to the task. If it's something we find important to be implemented, let's do it in another task.

What work remains for this task (it is set to 20% done)?

Greg Shah wrote:

What work remains for this task (it is set to 20% done)?

Is about removing the name_map.xml definitions where the LegacySignature can be used, for external programs, and change SourceNameMapper to load them from the annotations. And maybe also move the virtual definitions (IN SUPER, IN handle) to the converted .java code, and leave name_map.xml just a registry of all converted programs/.cls files.

Based on #6407-25 and #6407-1, the remaining work on this task should be centered around removing data from the name_map.xml and introducing it into the java code using LegacySignature.

• @<rest-service/> nodes (plain nodes)
• @<path-mapping/> nodes (plain nodes)
• @<class-mapping></class-mapping> nodes that can be plain or contain other nodes <method-mapping>[<parameter/>]</method-mapping> or/and just <parameter/>.

I found this wiki about the contents of the name_map.xml file: https://proj.goldencode.com/projects/internal-systems/wiki/Parameter_Passing.

path-mapping is something I added in #6649 as part of the SourceNameMapper improvements and were never documented. path-mapping was used to lookup and cache all filenames used in RUN statements, dynamic functions, new objects at server startup and avoid running getExternalProgram or getInternalEntry multiple times for the same program.

If the plan is to remove data from the name_map.xml and use annotations, then path-mapping is a good start as it is still in its early days.

You can't just remove them, the synthetic IN SUPER/handle/etc entries need to be registered somehow (if we get rid of them here, then we need them emitted in the converted program annotations).

Constantin Asofiei wrote:

You can't just remove them, the synthetic IN SUPER/handle/etc entries need to be registered somehow (if we get rid of them here, then we need them emitted in the converted program annotations).

I'd rather have them emitted in the converted program annotations and can be loaded when the ExternalProgram is build based on the procedure name. I will look into testcases to find an example and investigate.

Dănuț Filimon wrote:

Constantin Asofiei wrote:

You can't just remove them, the synthetic IN SUPER/handle/etc entries need to be registered somehow (if we get rid of them here, then we need them emitted in the converted program annotations).

I'd rather have them emitted in the converted program annotations and can be loaded when the ExternalProgram is build based on the procedure name. I will look into testcases to find an example and investigate.

Look in the official documentation for FUNCTION and PROCEDURE statements in 4GL. You can build the tests on this.

Otherwise, to emit them: I think we should do it at the class annotations. Note that OO .cls files also can define 4GL FUNCTIONS (IN handle/SUPER I think).

But, keep in mind: name_map.xml still needs to remain as a registry of all .cls and 4GL programs being converted. We do not want to rely on loading the .class files to build what is available in the converted application. Also, the loading from annotations should be done on in a lazy mode, if not already (I think for .cls is already done like this?).

  <class-mapping jname="support_6407.Test1Program" pname="support-6407/test1.p">
    <method-mapping jname="test1procedure" pname="test1procedure" type="PROCEDURE"/>
    <method-mapping in-super="true" jname="testf1" pname="testf1" returns="INTEGER" type="FUNCTION">
      <parameter jname="parm1" mode="INPUT-OUTPUT" pname="parm1" type="INTEGER"/>
    </method-mapping>
    <method-mapping in-super="true" jname="testf2" pname="testf2" type="PROCEDURE"/>
  </class-mapping>

The first method mapping is already annotated as

 @LegacySignature(type = Type.PROCEDURE, name ="test1procedure") 

   @LegacySignature(type = Type.FUNCTION, name = "testf1", returns = "INTEGER", parameters = 
   {
      @LegacyParameter(name = "parm1", type = "INTEGER", mode = "INPUT-OUTPUT")
   })

@LegacySignature(type = Type.PROCEDURE, name = "testf2")

testf1 and testf2 are part of another program, used through FUNC_CALL_SITE/RUN_CALL_SITE. The idea is to add class annotations as mentioned in #6407-115, but I don't think the LegacySignature contains the right properties to keep all the necessary data of the method-mapping (in-super for example).

Dănuț Filimon wrote:

testf1 and testf2 are part of another program, used through FUNC_CALL_SITE/RUN_CALL_SITE. The idea is to add class annotations as mentioned in #6407-115, but I don't think the LegacySignature contains the right properties to keep all the necessary data of the method-mapping (in-super for example).

Then we need to enhance the annotations. Can you sub-class Java annotation interfaces? I'd like to not pollute the normal LegacySignature with these in-super/in-handle/etc fields.

Another thought: it doesn't matter that the implementation is in some other program. The definition needs to be properly registered via annotatios.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

testf1 and testf2 are part of another program, used through FUNC_CALL_SITE/RUN_CALL_SITE. The idea is to add class annotations as mentioned in #6407-115, but I don't think the LegacySignature contains the right properties to keep all the necessary data of the method-mapping (in-super for example).

Then we need to enhance the annotations. Can you sub-class Java annotation interfaces? I'd like to not pollute the normal LegacySignature with these in-super/in-handle/etc fields.

Another thought: it doesn't matter that the implementation is in some other program. The definition needs to be properly registered via annotatios.

Java annotations can't be extended or subclassed. The only option is adding another annotation.

Dănuț Filimon wrote:

Java annotations can't be extended or subclassed. The only option is adding another annotation.

Thanks for checking. Then look into the syntax of name_map.xml and add to LegacySignature as needed.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Java annotations can't be extended or subclassed. The only option is adding another annotation.

Thanks for checking. Then look into the syntax of name_map.xml and add to LegacySignature as needed.

• boolean in-super
• String forward
• String pname
• String jname
• boolean private
• boolean in-handle
• int extent
• String qualified
• String libname
• String map-to
• String returns
• String mode
• String type

Most of the method information is added to the NameMappingWorker in collect_names.rules which is the annotations phase (pretty early). The good part is that this information is only added and not reused so we should be able to modify the code and include the same information when the LegacySignature is created for a function/procedure.

First, make the runtime work without method-mapping/parameter.

Constantin Asofiei wrote:

We do not want to rely on loading the .class files to build what is available in the converted application. Also, the loading from annotations should be done on in a lazy mode, if not already (I think for .cls is already done like this?).

ExternalProgram will be loaded based on what is being searched, but I do not see an alternative where the class is not loaded and the annotations checked.

Dănuț Filimon wrote:

Most of the method information is added to the NameMappingWorker in collect_names.rules which is the annotations phase (pretty early). The good part is that this information is only added and not reused so we should be able to modify the code and include the same information when the LegacySignature is created for a function/procedure.

First, make the runtime work without method-mapping/parameter.

Constantin Asofiei wrote:

We do not want to rely on loading the .class files to build what is available in the converted application. Also, the loading from annotations should be done on in a lazy mode, if not already (I think for .cls is already done like this?).

ExternalProgram will be loaded based on what is being searched, but I do not see an alternative where the class is not loaded and the annotations checked.

Exactly this is what I mean. We load the .class when that program gets searched.

• Add the properties from #6407-120 to LegacySignature (maybe LegacyParameter will require some additions too).
• Follow the conversion and check which functions/methods need to be moved as a class annotation (IN SUPER) and those that have to be updated with the new attributes.

Noticed that the getMethods() for a class returns execute() which is not part of the name_map.xml.

I am thinking of reusing the NameMappingWorker information to build what is necessary for the LegacySignature. This will make things a bit simpler for the moment until I can grasp a few more examples.

The reading of the name_map.xml and also LegacySignature is done in SourceNameMapper. There are the ExternalProgram and InternalEntry types which record this.

Look also in buildLegacyClass how this is done for converted .cls. The approach will be the same.

Regarding tests: look into the official syntax of the PROCEDURE and FUNCTION statements, including parameter definition for i.e. DLL arguments, and build tests from that; it shouldn't be hard, the tests just need to compile (for now).

We already have a lot of test code for DLL calls, it just isn't migrated to ABLUnit yet. See testcases/library_calls/@.

which functions/methods need to be moved as a class annotation (IN SUPER)

What do you mean by this? IN SUPER is a way of defining a function that just satisfies the concept of a "forward function definition" in languages like C. It tells the code that the function will be found in some super-procedure. But it doesn't define which super-procedure and this something that could even be different super-procedures at different times, since it is a runtime concept.

Greg Shah wrote:

We already have a lot of test code for DLL calls, it just isn't migrated to ABLUnit yet. See testcases/library_calls/@.

which functions/methods need to be moved as a class annotation (IN SUPER)

What do you mean by this? IN SUPER is a way of defining a function that just satisfies the concept of a "forward function definition" in languages like C. It tells the code that the function will be found in some super-procedure. But it doesn't define which super-procedure and this something that could even be different super-procedures at different times, since it is a runtime concept.

What we need here is to record (outside of name_map.xml) all these definitions which are required by runtime to resolve the real implementation. Now they exist in name_map.xml. The goal is to leave name_map.xml only as a registry of Java class names to the their 4GL class/program name.

I completed the tests for FUNCTION (IN SUPER, MAP TO, PRIVATE, FORWARD), I will make use of testcases/library_calls/ for future tests.

• procedure_definitions:159
• function_definitions:118

And I will go on with my initial idea of keeping the information in the NameMappingWorker and using it to build the annotations in those two places. Similar to buildLegacyClasses(), I will use buildExternalProgram() to create an alternative and make use of the LegacySignature information that is stored.

Dănuț Filimon wrote:

path-mapping is something I added in #6649 as part of the SourceNameMapper improvements and were never documented. path-mapping was used to lookup and cache all filenames used in RUN statements, dynamic functions, new objects at server startup and avoid running getExternalProgram or getInternalEntry multiple times for the same program.

If the plan is to remove data from the name_map.xml and use annotations, then path-mapping is a good start as it is still in its early days.

To get back on this, path-mapping can be the last in terms of being removed. Right now, we need it for the SourceNameMapper cache and it is possible to have it as a LegacyMapping at class level in the future.

The priority right now is to reduce the information stored in the class-mapping and I am currently working on this.

Constantin, if I have to build the InternalEntry from a LegacySignature of a method executed through a handle, is there a simple way of getting my hands on the method?

@LegacySignature(type = Type.FUNCTION, name = "ftest12", returns = "INTEGER", javaName = "ftest12", inHandle = true, mapTo = "ftest11")
@LegacySignature(type = Type.FUNCTION, name = "ftest10", returns = "INTEGER", javaName = "ftest10", inHandle = true, mapTo = "ftest9")
@LegacySignature(type = Type.FUNCTION, name = "ftest8", returns = "INTEGER", javaName = "ftest8", inHandle = true)
public class FunctionTest
{

Dănuț Filimon wrote:

Constantin, if I have to build the InternalEntry from a LegacySignature of a method executed through a handle, is there a simple way of getting my hands on the method?
[...]

There is no Java method associated in the InternalEntry for these signatures - this is resolved by the runtime.

The LegacySignature is not repeatable, so I'll have to create a container annotation for it.

I am having a bit of trouble creating a container annotation for LegacySignature, I am looking at something similar to DOC_COMMENT, but for the annotation container.

Dănuț Filimon wrote:

I am having a bit of trouble creating a container annotation for LegacySignature, I am looking at something similar to DOC_COMMENT, but for the annotation container.

Looked at DOC_COMMENT, I will create a CLASS_ANNOTATIONS in JavaTokenTypes and use it as an anchor.

Dănuț Filimon wrote:

Dănuț Filimon wrote:

I am having a bit of trouble creating a container annotation for LegacySignature, I am looking at something similar to DOC_COMMENT, but for the annotation container.

Looked at DOC_COMMENT, I will create a CLASS_ANNOTATIONS in JavaTokenTypes and use it as an anchor.

I found DatabaseReferences which is an even better example, I will try something similar!

The annotation looks like this now, which is better and it makes it easier to add new annotations.

@LegacySignatures(signatures = 
{
   @LegacySignature(type = Type.FUNCTION, name = "ftest12", returns = "INTEGER", javaName = "ftest12", inHandle = true, mapTo = "ftest11"),
   @LegacySignature(type = Type.FUNCTION, name = "ftest10", returns = "INTEGER", javaName = "ftest10", inHandle = true, mapTo = "ftest9"),
   @LegacySignature(type = Type.FUNCTION, name = "ftest8", returns = "INTEGER", javaName = "ftest8", inHandle = true)
})
public class FunctionTest
{

The parameters need also to be emitted - if you don't have yet, please add them.

Constantin Asofiei wrote:

The parameters need also to be emitted - if you don't have yet, please add them.

Added the parameters, functions should not be an issue now. I switched the priority to the annotations for PROCEDUREs now.

PROCEDURE proc-name 
  {   EXTERNAL "dllname" [ CDECL | PASCAL | STDCALL ]
         [ ORDINAL n ][ PERSISTENT ][ THREAD-SAFE ]
    | IN SUPER } :
  [ procedure-body ]

Wrote a few procedures that use THREAD-SAFE, but those failed the conversion. It looks like THREAD-SAFE is not supported. Constantin, do you know anything about this?

How are procedures that use EXTERNAL handled? Those do not appear in the converted code.

Dănuț Filimon wrote:

How are procedures that use EXTERNAL handled? Those do not appear in the converted code.

They appear only in name_map.xml and registered via SourceNameMapper.

Conversion is working properly, I checked the runtime for issue and compared the SourceNameMapper static members. Found the expected additions, but the InternalEntry "parameters" attribute is missing. Currently fixing this, then I will clean the implementation.

Rebased 6407b to latest trunk/16446, the branch is now at revision 16447.

A procedure file can have a parameter and this part is still being generated in the name_map.xml, currently working on removing it. I expect it to looks like this:

@LegacySignatures(parameters = 
{
   @LegacyParameter(name = "v1", jname = "v1_1", type = "INTEGER", mode = "INPUT")
},
signatures = 
{

})

Dănuț Filimon wrote:

A procedure file can have a parameter and this part is still being generated in the name_map.xml, currently working on removing it. I expect it to looks like this:
[...]

The LegacyParameter is added to the execute method, I am comparing 6407b and trunk to check how to use this parameter.

I've been trying to setup and run testcase and had a few minor issues, I took the testcases/library_calls/ and moved to a hotel copy. I rand the tests, but it looks like some of them are looking for windows paths.

I used the library_calls/test_runner.p for testing.

Dănuț Filimon wrote:

I've been trying to setup and run testcase and had a few minor issues, I took the testcases/library_calls/ and moved to a hotel copy. I rand the tests, but it looks like some of them are looking for windows paths.

I used the library_calls/test_runner.p for testing.

I found the readme.txt, it explains everything I need. I will check if my Windows virtual machine still works.

I've run the library_tests on Linux and there seems to be more LOAD/UNLOAD actions with 6407b then with trunk. The results look the same otherwise, but an extent regression from trunk prevents the output files from being generated. I will work on identifying this revision and reverting this change to check if I can get the output files.

I reverted till revision trunk/16200 and the output files for library_calls were not generated.

Danut,
Is Indeterminate extent dimension 0 is out of range 1 to 28000 raised?

Artur Școlnic wrote:

Danut,
Is Indeterminate extent dimension 0 is out of range 1 to 28000 raised?

3 times in total.

It looks like a regression, a library procedure is taking an output parameter size and is supposed to return a non 0 value, it returns ?, not sure why.

Looking into rest-service, it is related to name_map_merge. Is the name_map_merge parameter from p2j.cfg.xml used by any customer? I am wondering if we can just move this configuration to the directory.xml, similar to what was done in #11213.

Is the name_map_merge parameter from p2j.cfg.xml used by any customer?

I don't think so. Is it even needed?

Constantin?

Greg Shah wrote:

Is the name_map_merge parameter from p2j.cfg.xml used by any customer?

I don't think so. Is it even needed?

Constantin?

The ChUI app uses it.

Constantin Asofiei wrote:

Greg Shah wrote:

Is the name_map_merge parameter from p2j.cfg.xml used by any customer?

I don't think so. Is it even needed?

Constantin?

The ChUI app uses it.

There are also a few class-mapping nodes in this merge file, why is it necessary?

Dănuț Filimon wrote:

There are also a few class-mapping nodes in this merge file, why is it necessary?

What I mean to say is that the class-mapping nodes in the merge file will not be benefiting from the changes in this branch (it will still have methods, parameters and so on). If possible, I would like to get rid of this file and for that I need to understand why those class-mapping and rest-services are separated.

Dănuț Filimon wrote:

What I mean to say is that the class-mapping nodes in the merge file will not be benefiting from the changes in this branch (it will still have methods, parameters and so on). If possible, I would like to get rid of this file and for that I need to understand why those class-mapping and rest-services are separated.

This is hand-written Java code which defines either 4GL programs or REST services. It still needs to include the class-mapping nodes, but the signatures can be removed (or otherwise moved to the .java defs) as FWD should load them properly.

We certainly should move as much as possible to hand-coded annotations for these Java classes.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

What I mean to say is that the class-mapping nodes in the merge file will not be benefiting from the changes in this branch (it will still have methods, parameters and so on). If possible, I would like to get rid of this file and for that I need to understand why those class-mapping and rest-services are separated.

This is hand-written Java code which defines either 4GL programs or REST services. It still needs to include the class-mapping nodes, but the signatures can be removed (or otherwise moved to the .java defs) as FWD should load them properly.

I'll have to rewrite the java code so that I can remove the parameters, maybe I can replace the rest-service node with a class-mapping and add a isRest property so that it can make the difference between it being a class-mapping and a rest service.

I discussed with Greg and we already have a feature which support multiple name_map.xml files. The merge feature is unnecessary, since we can just have this name_map.xml file separately and it will be loaded like the converted one. I will go over this task again and check how I can use it.

Code Review Task Branch 6407b Revisions 16448 and 16449

The changes seem fine but I'll note that there are now some dead (unused) variables in annotations.xml.

ChUI runtime failed fast, I reconverted to retest, but the result was the same.

Fixed a scenario, retested runtime and found another issue which I am investigating now. It is similar to the first one.

ChUI tests failed. NOT_RUN - 535, PASSED - 45, FAILED - 276. The error I get is Procedure <> passed parameters to <>, which did not expect any. (1005) and I am trying to find a test case for this.

Dănuț Filimon wrote:

ChUI tests failed. NOT_RUN - 535, PASSED - 45, FAILED - 276. The error I get is Procedure <> passed parameters to <>, which did not expect any. (1005) and I am trying to find a test case for this.

The testcase is pretty simple, it involves a procedure that calls itself.

After the last test, I only have 3 remaining failed tests which I need to investigate (from the expected 3).

• annotations.xml - please remove all unused vars related to name_map_merge.xml (like mergeIndex, mergeRoot, etc)
• why do we need jname at the annotation? The annotation belogs to the Java method, so that's known.
  • this affects function_definitions.rules, method_definitions.rules, internal_procedures.rules
  • LegacyParameter also has jname annotation - for i.e. native procedures, there isn't even a java name associated. This also can be resolved (if needed) from the Java method parameters. But please explain why this is needed for parameters.
• why do we need the isPrivate flag at the annotation - this can too be solved from the Java method def. See function_definitions.rules and internal_procedures.rules. See also LegacySignature
• NameMappingWorker - programs map was made static. This will cause problems when doing runtime conversion. It needs to be in a WorkArea and context-local
• SourceNameMapper
  • buildClassExternalProgram - this does not implement the concept of 'delayed initialization', as we do for converted .cls files. We must not do Class.forName for the entire application converted programs, as FWD server initialization. This will both delay the FWD server startup and also load unnecessary/never executed code. The right approach is when resolving the Java class for a 4GL style program - at that point, if is not yet loaded, resolve all annotations.
  • buildInternalEntry has no javadoc for the return value
  • the rest-service functionality remains unchanged, right?

Dănuț Filimon wrote:

jname and private annotations appear in the name_map.xml of a customer project, so having them in LegacySignature is necessary.

Where are these emitted by conversion thus making LegacySignature requiring them?

Constantin Asofiei wrote:

Dănuț Filimon wrote:

jname and private annotations appear in the name_map.xml of a customer project, so having them in LegacySignature is necessary.

Where are these emitted by conversion thus making LegacySignature requiring them?

In collect_names.rules:

<action>methodRoot = nmap.addMethod(mapRoot, nameP, nameJ)</action> <!-- uses the javaname annotation for nameJ -->

         <!-- save the PRIVATE setting -->
         <rule>downPath(this, prog.kw_private) or downPath(this, prog.kw_proc, prog.kw_private)
            <action>
               nmap.putAttribute(methodRoot, "private", "true")
            </action>
         </rule>

• in method_definitions.rules, this is emitted at the annotation, but SourceNameMapper.buildLegacyClass uses the actual Java method name
• LegacyParameter annotation never required jname, why is it needed now?
• NativeAPIEntry has a jname, but that has no real purpose - there is no Java method for it.
• an internal procedure can't be defined both private and in super, in handle, native, etc. So there is no need for private to exist at the annotations, it can be inferred from the Java method.
• the same for jname at LegacySignature - even if the procedure is in super, the jname has no sense, as the actual target will be computed when the implementation is resolved. So for these, jname can be null and otherwise inferred from the actual Java method, if the annotation is for an implemented procedure/function/etc

The reason for the above is I want to make the annotations as minimal as possible.

Constantin Asofiei wrote:

Danut, I still don't understand what cases jname and private are needed. For example:

• in method_definitions.rules, this is emitted at the annotation, but SourceNameMapper.buildLegacyClass uses the actual Java method name
• LegacyParameter annotation never required jname, why is it needed now?
• NativeAPIEntry has a jname, but that has no real purpose - there is no Java method for it.
• an internal procedure can't be defined both private and in super, in handle, native, etc. So there is no need for private to exist at the annotations, it can be inferred from the Java method.
• the same for jname at LegacySignature - even if the procedure is in super, the jname has no sense, as the actual target will be computed when the implementation is resolved. So for these, jname can be null and otherwise inferred from the actual Java method, if the annotation is for an implemented procedure/function/etc

The reason for the above is I want to make the annotations as minimal as possible.

The nmap already stored the "jname" and "private" attributes, the name_map.xml was read and built the in-super/in-handle methods from it. The idea is that we have to be able to build both the methods defined in the class and those that are not defined (LegacySignatures).

We used jname and private in name_map.xml, when it was loaded by SourceNameMapper, because we didn't have the Java method resolved (and the Java class loaded) when this was done. With your changes, the annotations and Java method/class are loaded when we build this, so these are redundant to keep in LegacySignature converted code.

For LegacySignatures case - private can't happen, and the Java method name is really not needed - the actual implementation's Java method name is needed.

Dănuț Filimon wrote:

I see, it makes sense now. I'll have to change the method to parse the methods first and those that come from LegacySignatures to check for the associated method that was already parsed.

I don't understand - LegacySignatures is just 4GL names, you don't need Java names.

Committed 6407b/16452. Removed remaining usage of jname and private, ran a conversion with ChUI and found no issues. I'll also run ChUI runtime tonight to check for issues.

Dănuț Filimon wrote:

Committed 6407b/16452. Removed remaining usage of jname and private, ran a conversion with ChUI and found no issues. I'll also run ChUI runtime tonight to check for issues.

I don't see this revision.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Committed 6407b/16452. Removed remaining usage of jname and private, ran a conversion with ChUI and found no issues. I'll also run ChUI runtime tonight to check for issues.

I don't see this revision.

Oops, not bound. Should be available now.

Dănuț Filimon wrote:

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Committed 6407b/16452. Removed remaining usage of jname and private, ran a conversion with ChUI and found no issues. I'll also run ChUI runtime tonight to check for issues.

I don't see this revision.

Oops, not bound. Should be available now.

Did you push the branch? Do bzr unbind, bzr push --overwrite, then bzr bind again.

Constantin Asofiei wrote:

Did you push the branch? Do bzr unbind, bzr push --overwrite, then bzr bind again.

Just did it, can you check now?

Thanks. The only part that is missing is synchronization for ensureAnnotationsLoaded; and document in initializeAnnotations that is synchronized at the caller.

Stefanel reported the following issue in a customer application:

com.goldencode.p2j.cfg.ConfigurationException: Failed to initialize hook com.goldencode.p2j.main.StandardServer$16
   at com.goldencode.p2j.main.ServerHookManager.hookInitialize(ServerHookManager.java:170)
   at com.goldencode.p2j.main.StandardServer.bootstrap(StandardServer.java:1188)
   at com.goldencode.p2j.main.ServerDriver.start(ServerDriver.java:566)
   at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:522)
   at com.goldencode.p2j.main.ServerDriver.process(ServerDriver.java:233)
   at com.goldencode.p2j.main.ServerDriver.main(ServerDriver.java:1062)
   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.main.FwdLauncher.main(FwdLauncher.java:106)
Caused by: java.lang.NullPointerException: Cannot invoke "com.goldencode.p2j.util.InternalEntry.getMethod()" because "extProg.main" is null
   at com.goldencode.p2j.util.SourceNameMapper.registerServices(SourceNameMapper.java:3284)
   at com.goldencode.p2j.util.SourceNameMapper.registerServices(SourceNameMapper.java:579)
   at com.goldencode.p2j.main.StandardServer$16.initialize(StandardServer.java:1779)
   at com.goldencode.p2j.main.ServerHookManager.hookInitialize(ServerHookManager.java:147)
   ... 10 more

I committed a one line fix in 6407b/16454 to ensure the annotations are loaded when registering services.

Stefanel still reported a StackOverflowException after testing this fix. Although it did not look like a result of the tested changes, I will need to test the customer application locally and identify the issue myself to confirm.

Constantin, I found Stateaware.java in trunk (from webspeed) which needs to use LegacySignatures (just like the java written code from ChUI). This is a converted ADE file which was added to trunk, the customer project Stefanel creates an ExternalProgram for this class (and many others).

How should I proceed regarding this?

Dănuț Filimon wrote:

Constantin, I found Stateaware.java in trunk (from webspeed) which needs to use LegacySignatures (just like the java written code from ChUI). This is a converted ADE file which was added to trunk, the customer project Stefanel creates an ExternalProgram for this class (and many others).

How should I proceed regarding this?

Ouch... those need to be manually added. If is too complicated to do this manually, I think best way is to reconvert them and manually add the signatures.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin, I found Stateaware.java in trunk (from webspeed) which needs to use LegacySignatures (just like the java written code from ChUI). This is a converted ADE file which was added to trunk, the customer project Stefanel creates an ExternalProgram for this class (and many others).

How should I proceed regarding this?

Ouch... those need to be manually added. If is too complicated to do this manually, I think best way is to reconvert them and manually add the signatures.

Depends on how many I have to add haha. Do you have any tips on how should I approach this? Convert everything according to the ade project from #9488?

Dănuț Filimon wrote:

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin, I found Stateaware.java in trunk (from webspeed) which needs to use LegacySignatures (just like the java written code from ChUI). This is a converted ADE file which was added to trunk, the customer project Stefanel creates an ExternalProgram for this class (and many others).

How should I proceed regarding this?

Ouch... those need to be manually added. If is too complicated to do this manually, I think best way is to reconvert them and manually add the signatures.

Depends on how many I have to add haha. Do you have any tips on how should I approach this? Convert everything according to the ade project from #9488?

• scanning trunk for any InvokeConfig usage (to check if it is used in other converted ADE code)
• converted the program using the ade project from #9488
• meld and only moving the LegacySignatures, but I have to double check the code matches the methods used.

Danut, check the name_map.xml in the FWD project - all in super/etc are there.

Constantin Asofiei wrote:

Danut, check the name_map.xml in the FWD project - all in super/etc are there.

The name_map.xml from the p2j.jar, thanks!

I've identified the following:

web/objects/stateaware.p
webutil/webstart.p
webutil/_relname.p
webutil/debug.p
web/support/weblist.p
web/support/webinput.p
web/support/webedit.p
web/support/tagrun.p
web/support/webmsg.p
web/objects/stateaware.p
web/objects/web-util.p
web/objects/web-disp.p
web/objects/session.p
adecomm/_osfext.p
adecomm/_relname.p
adecomm/_rsearch.p
web/support/printval.p
web/support/tagparse.p
webutil/e4gl-gen.p
web/support/webradio.p
web/support/webtog.p

• stateaware.p, there are a lot of in-handle mapping nodes that are not added to the signatures (conversion issue), web-disp.p which defined the same methods has those nodes - currently investigating
• web/objects/web-util.p is also missing 4 LegacySignature

Constantin, do you know how the trunk/build/classes/com/goldencode/p2j/name_map.xml is generated? It is supposed to be manually updated from the contained text, but it seems to come from somewhere.

Dănuț Filimon wrote:

Constantin, do you know how the trunk/build/classes/com/goldencode/p2j/name_map.xml is generated? It is supposed to be manually updated from the contained text, but it seems to come from somewhere.

Is from src/com/goldencode/p2j/name_map.xml. Also, this needs to be updated to remove all internal entry signatures and leave only the registered program.

I found a scenario where the java name of the method is needed and it must be added to the LegacySignature.

A program defines a FUNCTION through the handle of a persistent procedure, the function is registered in the execute method :

 ProcedureManager.registerFunctionHandle("logNote", webUtilitiesHdl);

   @LegacySignature(type = Type.FUNCTION, name = "logNote", returns = "LOGICAL", inHandle = true, parameters = 
   {
      @LegacyParameter(name = "pcLogType", type = "CHARACTER", mode = "INPUT"),
      @LegacyParameter(name = "pcLogText", type = "CHARACTER", mode = "INPUT")
   }),

The code reaches SourceNameMapper.initializeAnnotations() and the method is null (it does not appear in the converted code), the line assigns "" to the iejname:

String iejname = (method != null) ? method.getName() : "";

j2p.put(ie.jname, ie);

It seems we can't escape from not using the java name in the annotations.

I don't understand. the jname is of no use until the real implementation is resolved.

Constantin Asofiei wrote:

I don't understand. the jname is of no use until the real implementation is resolved.

We are populating the ExternalProgram.j2pf map with "" keys instead of the actual jname, the name_map.xml contains the jname for this kind of situations. We do not have any method to "resolve" the jname to.

Dănuț Filimon wrote:

Constantin Asofiei wrote:

I don't understand. the jname is of no use until the real implementation is resolved.

We are populating the ExternalProgram.j2pf map with "" keys instead of the actual jname, the name_map.xml contains the jname for this kind of situations. We do not have any method to "resolve" the jname to.

Please create a standalone test which shows this problem in 6407b

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin Asofiei wrote:

I don't understand. the jname is of no use until the real implementation is resolved.

We are populating the ExternalProgram.j2pf map with "" keys instead of the actual jname, the name_map.xml contains the jname for this kind of situations. We do not have any method to "resolve" the jname to.

Please create a standalone test which shows this problem in 6407b

I found it in ADE abl/src/web/objects/stateaware.p, but I'll try to make a smaller test case and commit my current changes to 6407b.

I had to add this:

@@ -3886,7 +3889,7 @@
                {
                   executeSignature = ls;
                }
-               else
+               else if (!ls.type().equals(Type.EXECUTE))
                {
                   legacySignatures.add(ls);
                   signatureToMethods.put(ls, method);

• start.p
  

  define new global shared variable hdl as handle no-undo.
  function lognote returns logical (v1 as character) in hdl.
  function lognote1 returns logical (v1 as character) in hdl.
  

• other.p
  

  define new global shared variable hdl as handle no-undo.
  hdl = this-procedure.
  
  function lognote returns logical (v1 as character):
     return yes.
  end.
  
  function lognote1 returns logical (v1 as character):
     return no.
  end.
  

In SourceNameMapper.initializeAnnotations(), j2p.put(ie.jname, ie); uses "" as key two times and overrides lognote InternalEntry.

I also committed 6407b/16536 - The Tyep.EXECUTE methods do not have a name in the annotation that can be used, so I excluded them. There was no part where it was being necessary from debugging, so it will pose no problem.

Danut, sorry, but the runtime works properly. So the problem is just that extprog.j2pf is added an empty key - as there is no Java method (thus jname) for such signatures. IMO, just avoid adding them to j2pf map altogether.

Constantin Asofiei wrote:

Danut, sorry, but the runtime works properly. So the problem is just that extprog.j2pf is added an empty key - as there is no Java method (thus jname) for such signatures. IMO, just avoid adding them to j2pf map altogether.

And the same for j2pp maps.

Constantin Asofiei wrote:

Constantin Asofiei wrote:

Danut, sorry, but the runtime works properly. So the problem is just that extprog.j2pf is added an empty key - as there is no Java method (thus jname) for such signatures. IMO, just avoid adding them to j2pf map altogether.

And the same for j2pp maps.

If the runtime works fine, a check before adding to the j2pf/j2pp maps will do the trick. I made a logging file with the contents of the SourceNameMapper collections and compared it with a before/after, so not having all the information was a problem that needed to be investigated.

I will finish investigating #6407-203 and commit the converted changes (the LegacySignatures obtained from the ADE code conversion).

Danut, I was wrong in the meet: the LegacySignature needs to be there, but not the implementation.

See existing name_map.xml in FWD:

<method-mapping in-handle="true" jname="convertDatetime" pname="convert-datetime" returns="CHARACTER" type="FUNCTION">

So this is a bug in 6407b.

• for functions, on line 412:
  

              <!-- collect the copy, as we might need to hide it later -->
              <!-- conditional add: only one IN SUPER, IN handle or LOCAL
                 function allowed; first defined survives, all other get
                 hidden -->
              <rule on="false">downPath(copy, prog.kw_in)
                 <rule>downPath(copy, prog.kw_in, prog.kw_super)
                    <rule>!hmver.containsKey("in_super")
                       <action>hmver.put("in_super", copy)</action>
                       <action on="false">hide = true</action>
                    </rule>
  
                    <rule on="false">!hmver.containsKey("in_handle")
                       <action>hmver.put("in_handle", copy)</action>
                       <action on="false">hide = true</action>
                    </rule>
                 </rule>
  
                 <rule on="false">!hmver.containsKey("local")
                    <action>hmver.put("local", copy)</action>
                    <action on="false">hide = true</action>
                 </rule>
              </rule>
  

• for procedures, on line 366:
  

           <!-- collect the copy, as we might need to hide it later -->
           <!-- conditional add: only one IN SUPER and one LOCAL procedure
                allowed; first defined survives, all other get hidden -->
           <rule>downPath(copy, prog.kw_proc, prog.kw_in, prog.kw_super)
              <rule>!hmver.containsKey("in_super")
                 <action>hmver.put("in_super", copy)</action>
                 <action on="false">hide = true</action>
              </rule>
  
              <rule on="false">!hmver.containsKey("local")
                 <action>hmver.put("local", copy)</action>
                 <action on="false">hide = true</action>
              </rule>
           </rule>
  

The test needs to include multiple procedure/function with the same name and signature, one with IN SUPER/handle the other an actual implementation, and see how trunk and 6407b behaves.

For some reason the second one survives (when is an implementation) during conversion (which looks wrong).

Constantin Asofiei wrote:

Danut, I was wrong in the meet: the LegacySignature needs to be there, but not the implementation.

See existing name_map.xml in FWD:
[...]

So this is a bug in 6407b.

See these rules in fixups/functions_procedures.rules:

• for functions, on line 412:
  [...]
• for procedures, on line 366:
  [...]

The test needs to include multiple procedure/function with the same name and signature, one with IN SUPER/handle the other an actual implementation, and see how trunk and 6407b behaves.

For some reason the second one survives (when is an implementation) during conversion (which looks wrong).

I tested trunk and the function is also present alongside the one registered through the handle. I will work on a fix for this to proceed with the remaining work in 6407b.

Actually, it can't be dropped. There's something stupid where a function can be both in handle and implementation; see this:

def var h as handle.
function func0 returns int in h.

function func0 returns int.
message "here".
end.

h = this-procedure.
func0().

def var h as handle.
function func0 returns int in h.

function func0 returns int.
message "here".
end.

run ddf2.p persistent set h.

dynamic-function("func0"). // here 2

h = this-procedure.

dynamic-function("func0"). // here

function func0 returns int.
message "here 2".
end.

It does make sense to be able to change the handle by setting this-procedure. Regardless, both the LegacySignature and implementation should be present, let the runtime handle which one should be used.

Dănuț Filimon wrote:

Regardless, both the LegacySignature and implementation should be present, let the runtime handle which one should be used.

Yes, I think that's the conclusion. Please run this test with both trunk and 6407b.

Also, make sure to build a tool so you can compare internal entries which are in super/handle/dll/etc from name_map.xml with the ones in LegacySignatures.

I've had to replace the String key used for storing the methods in ProgramInfo, the problem was that name collection would override the name in #6407-217. I am doing a customer conversion to confirm everything works properly.

Constantin, should FORWARD functions be added to the LegacySignatures? I have a scenario with a FORWARD function that is not defined later in the code.

Dănuț Filimon wrote:

Constantin, should FORWARD functions be added to the LegacySignatures? I have a scenario with a FORWARD function that is not defined later in the code.

Was it in name_map.xml originally? Check also via this-procedure:INTERNAL-ENTRIES if it exists in the list or not.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin, should FORWARD functions be added to the LegacySignatures? I have a scenario with a FORWARD function that is not defined later in the code.

Was it in name_map.xml originally? Check also via this-procedure:INTERNAL-ENTRIES if it exists in the list or not.

It exists in the original name_map.xml of the converted code (customer application) and this-procedure:INTERNAL-ENTRIES is not used at all. I can add it just to be sure there are no issues in the future with this.

I committed 6407b/16537. I committed the LegacySignatures converted for the webspeed code, modified the rules to add the in-handle/in-super/DLL-ENTRY/forward defined to it.

The solution works properly with a customer conversion, but I am still wondering if it is possible to have the scenario from #6407-217 where there there's more than a function present in the procedure (both in-handle and in-super). If that's the case, MethodKey can be expanded to store an enum with the type.

Constantin, can you take a look at the changes and let me know what you think?

Danut, changes in 6407b look good. Please commit the tool to compare the annotations/name_map.xml to ~/secure/code/p2j_repo/tools/dev_helpers/<project>/ - each project should 'build' itself.

Constantin Asofiei wrote:

Danut, changes in 6407b look good. Please commit the tool to compare the annotations/name_map.xml to ~/secure/code/p2j_repo/tools/dev_helpers/<project>/ - each project should 'build' itself.

The tool requires an old and new name_map.xml (from the old and new conversion), I am not sure if it will be necessary in the future if we move on from method-mappings and parameter nodes.

Constantin Asofiei wrote:

Danut, please add history entry to WebUtil.java. Otherwise, is there more testing needed?

I managed to test ChUI and one customer conversion with no issues.

Dănuț Filimon wrote:

Constantin Asofiei wrote:

Danut, please add history entry to WebUtil.java. Otherwise, is there more testing needed?

I managed to test ChUI and one customer conversion with no issues.

Please test the remainder apps.

I got news from Serban that the runtime testing failed for a customer, I am currently investigating.
https://proj.goldencode.com/projects/regression/wiki/Cross-Customer_Test_Plan_Tracking

Please post a standalone test.

Constantin Asofiei wrote:

Please post a standalone test.

This is the test case.

define variable hpUtils as handle no-undo.
hpUtils = this-procedure.
function test1 returns character(input c1 as character) in hpUtils.

function test1 returns character(input c1 as character):
    return "1".
end function.

if test1("")="1":U then do:
    message "hji".
end.

• test1.p
  

  define variable hpUtils as handle no-undo.
  hpUtils = this-procedure.
  function test1 returns character(input c1 as character) in hpUtils.
  
  function test1 returns character(input c1 as character):
      return "1".
  end function.
  
  if test1("")="1":U then do:
      message "good1".
  end.
  
  run test2.p persistent set hpUtils.
  if test1("") = "2" then do:
     message "good2".
  end.
  

• test2.p
  

  function test1 returns character(input c1 as character):
      return "2".
  end function.
  

In trunk, the IN handle from the current procedure (if the implementation also exists for the function) gets lost. So the if test1("") = "2" then do: fails - as it loses information about the handle completely.

This is not about the jname, but about losing information; with 6407b, the in handle signature is registered in ieMap, while the implementation from current procedure gets lost.

• InternalEntryKey needs to have also as part of the key the IN handle flag (only this shows the problem)
• in ControlFlowOps$InternalResolver.resolve:
  • first with the IN handle flag set to true - to resolve the 'in handle' signature
  • if one was found and the handle is this-procedure instance, then do another lookup to get the implementation (with in handle flag to false)
  • if one was found and the handle is not this-procedure instance, then call recursively, as test2.p above can also have the signature IN handle (similar to how we manage else if (mapTo != null) case)

Please create a test for the last bullet-point.

Constantin Asofiei wrote:

• InternalEntryKey needs to have also as part of the key the IN handle flag (only this shows the problem)
• in ControlFlowOps$InternalResolver.resolve:
  • first with the IN handle flag set to true - to resolve the 'in handle' signature

I missed this step: if one was not found, then do a lookup with the flag set to false.

• if one was found and the handle is this-procedure instance, then do another lookup to get the implementation (with in handle flag to false)
• if one was found and the handle is not this-procedure instance, then call recursively, as test2.p above can also have the signature IN handle (similar to how we manage else if (mapTo != null) case)

Looks good. Please also test what happens if hpUtils in test2.p or in start.p is unknown.

Constantin Asofiei wrote:

Looks good. Please also test what happens if hpUtils in test2.p or in start.p is unknown.

• hpUtils = this-procedure. to hpUtils = ?.
  • OE:

    Could not evaluate the expression describing the context of external function 'test1'. (2767)

    Window, then

    good3
    good2

  • FWD
    

    26/05/21 14:14:17.050+0300 |  SEVERE | com.goldencode.p2j.main.StandardServer [StandardServer.invoke()] | ThreadName:Conversation [00000002:bogus], Session:00000002, ThreadId:00000006, User:bogus | Abnormal end!
    java.lang.RuntimeException: invoke() of class com.goldencode.hotel.Start and  method execute failed
        at com.goldencode.p2j.util.ControlFlowOps.invokeError(ControlFlowOps.java:8577)
        at com.goldencode.p2j.util.ControlFlowOps.invokeExternalProcedure(ControlFlowOps.java:6758)
        at com.goldencode.p2j.util.ControlFlowOps.invokeExternalProcedure(ControlFlowOps.java:6527)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:1381)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:968)
        at com.goldencode.p2j.main.StandardServer$LegacyInvoker.execute(StandardServer.java:2717)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:2101)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:716)
        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.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.ControlFlowOps$InternalEntryCaller.invokeImpl(ControlFlowOps.java:9802)
        at com.goldencode.p2j.util.ControlFlowOps$InternalEntryCaller.invoke(ControlFlowOps.java:9759)
        at com.goldencode.p2j.util.ControlFlowOps.invokeExternalProcedure(ControlFlowOps.java:6669)
        ... 15 more
    Caused by: java.lang.RuntimeException: invoke() of program test1 failed
        at com.goldencode.p2j.util.ControlFlowOps.invokeError(ControlFlowOps.java:8583)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:7727)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:4582)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:7229)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:7132)
        at com.goldencode.p2j.util.ControlFlowOps.invokeFunctionImpl(ControlFlowOps.java:7062)
        at com.goldencode.p2j.util.ControlFlowOps.invokeFunctionWithMode(ControlFlowOps.java:3820)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:1255)
        at com.goldencode.p2j.util.InvokeConfig.execute(InvokeConfig.java:448)
        at com.goldencode.hotel.Start.lambda$execute$0(Start.java:47)
        at com.goldencode.p2j.util.Block.body(Block.java:636)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:9723)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:9333)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:706)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:679)
        at com.goldencode.hotel.Start.execute(Start.java:42)
        ... 22 more
    Caused by: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "referent" is null
        at com.goldencode.p2j.util.ProcedureManager.getAbsoluteName(ProcedureManager.java:2177)
        at com.goldencode.p2j.util.ControlFlowOps$InternalResolver.resolve(ControlFlowOps.java:11470)
        at com.goldencode.p2j.util.ControlFlowOps$InternalResolver.resolve(ControlFlowOps.java:11585)
        at com.goldencode.p2j.util.ControlFlowOps$Resolver.resolve(ControlFlowOps.java:11447)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:7449)
        ... 36 more
    

• removed the test1 function definition from test2:
  • OE: The code builds and can be executed, the result is

    ** Unable to udnerstand after -- "test1". (247)
    ** test2.p Could not understand line 4. (198)

    Window, then

    good1

  • FWD: The conversion fails during scan phase, the converted code does not compile

    compile:
         [echo] JAVAC DMO and UI classes in /home/ddf/gcd/branches/hotel_gui/build/classes.aop
        [javac] Compiling 34 source files to /home/ddf/gcd/branches/hotel_gui/build/classes
         [echo] JAVAC all others from /home/ddf/gcd/branches/hotel_gui/src into /home/ddf/gcd/branches/hotel_gui/build/classes
        [javac] Compiling 5 source files to /home/ddf/gcd/branches/hotel_gui/build/classes
        [javac] /home/ddf/gcd/branches/hotel_gui/src/com/goldencode/hotel/Test2.java:32: error: illegal start of expression
        [javac]          if (;
        [javac]              ^
        [javac] 1 error

Dănuț Filimon wrote:

• hpUtils = this-procedure. to hpUtils = ?.

This needs to be fixed in FWD.

• removed the test1 function definition from test2:

test2 becomes invalid OE code, you can't compile it if there is no definition (IN super or other) in test2.p So this is not a valid scenario to test.

Committed 6407b/16577. Added 2767 error handling and fixed the remaining scenario where the function declaration is resolved to a different handle.

There was also an infinite loop caused by going through the super-procedures of the handle (and session), but that was solved by looking up for the InternalEntry using the SourceNameMapper.

Currently checking other tests from the same customer to confirm all of them work properly.

Danut, please create a standalone test for the 16577 commit.

The resolvers must not have dependencies on each other. I need to understand the test to see how it works with trunk.

Constantin Asofiei wrote:

Danut, please create a standalone test for the 16577 commit.

The resolvers must not have dependencies on each other. I need to understand the test to see how it works with trunk.

• start.p
  

  define variable hpUtils as handle no-undo.
  run test2.p persistent set hpUtils.
  function test1 returns character(input c1 as character) in hpUtils.
  
  message test1("").
  

• test2.p
  

  function test1 returns logical (input c1 as character):
     return yes.
  end.
  

I have some problems figuring out what is wrong with the changes from the last commit, the only difference I can find is that a different handle is used when initially resolving a specific method. One is using the handle of the current procedure and the other uses an internal handle, somehow the targetProcedure() call ends up giving the wrong handle when calling a dynamic function, the test case is hard to figure out.

Danut, please rebase the branch. Thanks.

Constantin Asofiei wrote:

Danut, please rebase the branch. Thanks.

Rebased 6407b to latest trunk/16653, the branch is now at revision 16668.