On Sun, 19 Mar 2023 09:07:41 GMT, Alan Bateman <[email protected]> wrote:
>> Hello Mandy, Alan, Jim,
>>
>> I've updated this PR to take into account these suggestions. I went ahead
>> with what Mandy suggested and enhanced the existing (internal)
>> `jdk.tools.jlink.internal.Platform` `record`to additional parse these
>> architectures. It was anyway doing it for a select few.
>>
>> In addition to that, I also moved the architecture to endianness mapping
>> into this `Platform` class itself. I followed Jim's suggestion to move the
>> mapping out of the code. The `Platform` code now reads this from a
>> `endianness.properties` file which is an internal resource of the
>> `jdk.jlink` module. However, I'm having second thoughts about this part. I'm
>> guessing that when Jim suggested moving this to a resource, it was probably
>> because the code resided within the `JLinkTask` class. Now that I've moved
>> this to an existing `Platform` class which is solely meant for things like
>> these, I feel that we could probably just hard code these architecture to
>> endianness mapping within this class itself, instead of reading it from a
>> properties file. I'm looking for some inputs on what you all think would be
>> the best way to proceed.
>>
>> Alan, as for the `osname-arch=little/big` vs `arch=little/big` mapping
>> style, I decided to use the `arch=little/big` and let the Platform class
>> parse the architecture out of the string in the `ModuleTarget`. I did it for
>> a couple of reasons:
>>
>> 1. The `Platform` class already has the necessary logic to do that, plus it
>> has to do that anyway (for us to implement the suggestion that Mandy noted
>> about using this in `DefaultImageBuilder`)
>>
>> 2. As far as I could see, the OS name shouldn't play any role in the
>> endianness, so leaving that out felt easier to deal with since we wouldn't
>> have to think of what OS name, arch combinations would be valid.
>>
>> If you or others feel that it's better to stick with the
>> `osname-arch=little/big` approach, instead of this one, please do let me
>> know and I'll go ahead and do that change.
>
>> If you or others feel that it's better to stick with the
>> osname-arch=little/big approach, instead of this one, please do let me know
>> and I'll go ahead and do that change.that out felt easier to deal with since
>> we wouldn't have to think of what OS name, arch combinations would be valid.
>
> My preference would be something like target.properties so we have one place
> with the properties for target platform when cross linking.
> ${ModuleTarget}.${property} could work as keys. At some point we will need to
> extend or replace the ModuleTarget as it too basic to describe the target
> platform, that's why I was hoping to avoid parsing it here.
>
> jlink cannot be reliably used to cross link to target a different JDK
> release. Reasons include changes to the run-time image, jimage changes, and
> plugins that are deeply tied to java.base or other modules. I mention this
> because JDK 11 was the last release to build solaris-sparcv9, so I think you
> can this and several others from the properties file. Probably just stick the
> the builds that are possible in the main line.
Thank you Alan for these additional inputs. I've now updated the PR to use a
`target.properties` to map the `osname-arch` to a endianness. The
implementation in `Platform` class will no longer use the arch substring to
decide the endianness and instead will use the complete `osname-arch` for
determining the endianness.
Additionally, I have trimmed down the number of `osname-arch` mapping to what I
think is the current supported ones in mainline. If this list is inaccurate
(which is possible), then please do let me know and I'll investigate further.
-------------
PR: https://git.openjdk.org/jdk/pull/11943
