JDK-8178075 : Provide generic add-exports mechanism
  • Type: Bug
  • Component: javafx
  • Sub-Component: build
  • Affected Version: 10
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2017-04-04
  • Updated: 2017-04-28
  • Resolved: 2017-04-05
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 9
10Fixed 9Fixed
Related Reports
Blocks :  
JDK-8177566 - FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline
Description
When Jonathan was adding a new package to FX, we realized that he needed to add his:
    --add-export=javafx.graphics/xxxx=javafx.controls 
to 5 different files, 4 of them the generated argfiles in ./build

If we add a mechanism to concatenate the contents of a source file, for example buildSrc/addExports to the four generated argfiles, then we only need to modify one file when creating a new package, and modify one file when the change has been promoted into the JDK.

The task is to create the mechanism, starting with close an addExports file that contains comments to start.
Comments
+1 on the backport
28-04-2017
[~ddhill] Please review the backport request. The changeset patch applies cleanly, so I think there is no need for a new webrev. The pointer to the changeset patch is as noted above: http://hg.openjdk.java.net/openjfx/10-dev/rt/rev/834498972ddf I plan to do an "hg export 834498972ddf" in 10-dev followed by "hg import" in 9-dev. I tested this on all three platforms today during my weekly sanity testing. I also verified that the production bits produced before and after this patch are identical.
28-04-2017
I note that the patch applies cleanly to FX 9-dev and I am able to test the fix for JDK-8177566 with that patch applied. I will do more testing and then request the backport to JDK 9. Since this does not touch the product bits, we don't need a formal "jdk9-fix-request".
28-04-2017
Raising the priority to P2, since this effectively blocks our ability to test any bug fix that requires adding qualified exports from one module to another. We now have such a fix in progress, JDK-8177566, so this will block that bug fix. As such we need to backport this to JDK 9. This is a safe build fix that only affects scripts used by developers in building, running, and testing FX before delivering it to the JDK. It does not touch the product bits we deliver.
27-04-2017
Changeset: 834498972ddf Author: ddhill Date: 2017-04-05 18:51 -0400 URL: http://hg.openjdk.java.net/openjfx/10-dev/rt/rev/834498972ddf 8178075: Provide generic add-exports mechanism Reviewed-by: kcr, jgiles ! build.gradle + buildSrc/addExports
05-04-2017
I applied the patch and only edited the new addExports file. I was able to run all unit tests in javafx.graphics and javafx.controls with my new API, and I was also able to run a Hello app without anything more than the run.args file. Everything seems to run perfectly as far as I can see.
05-04-2017
Looks good to me. I tested it and it behaves as I would expect. One small typo to fix in your comments: s/dependant/dependent/ No need for a new webrev. +1
05-04-2017
webrev: http://cr.openjdk.java.net/~ddhill/8178075.3
05-04-2017
> + line = line.replace('--add-exports *', '--add-exports=') you should have ' *' (that is SPACE-SPACE-ASTERISK) otherwise this will transform the following: --add-exports=module/package=targetmodule to: --add-exports==module/package=targetmodule [edited to add] although having said that, it doesn't seem to fail in the way I expected it to. OK, actually with the change the replacement doesn't happen at all.
05-04-2017
new version. I think this one will mean just one addition - to buildSrc/addExports Of note, I will only add the buildSrc/addExports to the args files if it is non-empty (stripped of comments). One lasting concern, we have some side builds that possibly could need additions. This would only be the case if they are not using a proper module-source-path that contains the affected modules. I am not aware of any of these, but I may have missed one. For those, there is a variable set that is currently unused, that can be used to add the extras in easily. Did what testing I could think of, the real test will be contact with the real world effort, so I would like to test this against Jonathan's working changes. webrev: http://cr.openjdk.java.net/~ddhill/8178075.2
05-04-2017
The current version looks good to me, although I agree that it would be nicer if you only had to add them in the one place. I would expect that it would be needed in three places currently: buildSrc/addExports, modules/*/src/test/addExports, and build.gradle (if another module depends on the newly added package).
05-04-2017
@Jonathan, I agree that we need to mark anything that is temporary with a JBS number as a reminder to pull it out again, and to provide context if we forget. We can either wiki the process or I can beef up the comment in the buildSrc/addExports file, was leaning towards the first. In regards to needing it in two places.... that surprised me until I pulled some memories out of storage. Our javac of the modules don't directly use the args files, which are generated for certain ant based tests and to support our developers. In effect the arg files are writing out some internal variables for later use. Could you email me your working patch so I can fix this "right". What I need to do is read in the buildSrc/addExports file to a variable early on (filtering comments) and then tacking it onto the options used for javac, as well as emitting the comments and all to the arg files.
05-04-2017
Woke up this morning and realized I did miss something, 2x + inputs.file(EXTRAADDEXPORTS); with these in place and build.gradle compiled, I am seeing no change in run.args when there is no change, and change when I modify buildSrc. webrev: http://cr.openjdk.java.net/~ddhill/8178075.1
05-04-2017
Another comment would be that we should have a standard style in the buildSrc/addExports file - so that we know why the exports are added, who added them, and for what purpose (so we don't end up with exports lingering by mistake).
05-04-2017
I applied this patch and tested. I was a little unsure what files this would touch, so I removed it from all my changes I made earlier. When I noted that it did not modify the src/test/addExports files, I added those lines back in, and everything works fine. I can see the args are added into the generated build/*.args files. Therefore, the comment I would have is to be clear in the wiki that there are now *two* locations where this editing is required - the buildSrc/addExports file, and the src/test/addExports files for the relevant modules. In my case, I need to do this for both javafx.graphics and javafx.controls, and then everything works. +1
05-04-2017
@ Kevin - at first glance - I would say we have an error and yes, we are rewriting all four arg files regardless (despite my efforts to stop that).
04-04-2017
Haven't tried it yet, but will this correctly rebuild the various args files if you do an incremental build where the only thing you change is the buildSrc/addExports file?
04-04-2017
webrev: http://cr.openjdk.java.net/~ddhill/8178075
04-04-2017