On Wed, 8 Nov 2023 16:22:31 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> Consider a simple module, like:
>>
>> module test {}
>>
>>
>> And compile it with JDK 22 and JDK 21 using:
>> javac --release 21
>>
>> The results of the compilations will differ: when compiling with JDK 21, the
>> mandated java.base dependency will get a version, possibly like
>> "21-internal". When compiling with JDK 22, the version of the java.base
>> dependency will be empty.
>>
>> This is a) because `module-info.class`es in `ct.sym` do not have any module
>> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather
>> `lib/modules`, which may contain a range of version specifiers.
>>
>> This patch does two changes:
>> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a
>> simple version. For `--release N`, the version is `N`.
>> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a
>> `lib/modules` is never used. I.e. the appropriate classfiles are copied into
>> `ct.sym`. This not only allows for a general approach to module versions,
>> but simplifies the `--release` handling in javac, and should enable future
>> improvements. This is, however, a relatively big change.
>>
>> The use of `lib/modules` for `--release <current>` was made to improve build
>> performance, but the build has been updated since this has been introduced,
>> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>>
>> With these changes, compiling with `--release N` should record the same
>> dependency versions in `module-info` on JDK N and JDK N + 1.
>
> make/modules/jdk.compiler/Gendata.gmk line 75:
>
>> 73: $(wildcard $(MODULE_SRC)/share/data/symbols/*) \
>> 74: $(MODULE_INFOS) \
>> 75: $(foreach m, $(CT_MODULES) $(call
>> FindTransitiveIndirectDepsForModules, $(CT_MODULES)), \
>
> This dependency line is hard to read. Maybe extract it to a variable and
> depend on the contents of the variable instead?
>
> Also, this is the only place where CT_MODULES is used. I'd recommend also
> creating an intermediate variable like CT_TRANSITIVE_MODULES for $(call
> FindTransitiveIndirectDepsForModules, $(CT_MODULES)), and grouping all these
> declarations in the same place (either in the top of the file as now, or
> closer to the usage here).
In fact, it took me some time to realize these were dependencies, and not lines
in the rule. Maybe if we also make a variable for the symbols/* dependencies,
we can fit all dependencies in a single line? That'd definitely help with
readability.
Alternatively, do not separate each dependency on a single line, but start them
directly after the colon, and just break lines when they no longer fit due to
line length.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1386893537
