Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags [v3]
> Currently the only other option for manually configuring the build platform > while cross compiling are devkits, which don't work on certain systems and > are also more focused on differentiating the build and target compilers > instead. This patch adds the ability to explicitly set the build platform > through a new option, which can be especially helpful for when autodetection > fails and devkits cannot be relied on, and also for simpler cross compilation > cases (Like the one described in building.md) > > This also removes support for the legacy cross compilation flags as well. > > WIP: Translation from markdown to html Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Don't perform autodetection if --build is specified - Changes: - all: https://git.openjdk.java.net/jdk/pull/7656/files - new: https://git.openjdk.java.net/jdk/pull/7656/files/d523d495..9189e54b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7656&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7656&range=01-02 Stats: 11 lines in 1 file changed: 7 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7656/head:pull/7656 PR: https://git.openjdk.java.net/jdk/pull/7656
Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags [v2]
> Currently the only other option for manually configuring the build platform > while cross compiling are devkits, which don't work on certain systems and > are also more focused on differentiating the build and target compilers > instead. This patch adds the ability to explicitly set the build platform > through a new option, which can be especially helpful for when autodetection > fails and devkits cannot be relied on, and also for simpler cross compilation > cases (Like the one described in building.md) > > This also removes support for the legacy cross compilation flags as well. > > WIP: Translation from markdown to html Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Adopt better solution for processing provided flags - Changes: - all: https://git.openjdk.java.net/jdk/pull/7656/files - new: https://git.openjdk.java.net/jdk/pull/7656/files/7082b8ba..d523d495 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7656&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7656&range=00-01 Stats: 33 lines in 1 file changed: 3 ins; 15 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/7656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7656/head:pull/7656 PR: https://git.openjdk.java.net/jdk/pull/7656
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]
On Wed, 2 Mar 2022 21:53:01 GMT, Tim Prinzing wrote:
>> The caller class returned by Reflection::getCallerClass was used to gain
>> access to it's module in most cases and class loader in one case. I added a
>> method to translate the caller class to caller module so that the decision
>> of what module represents the caller with no stack frame is made in a single
>> place. Calls made to caller.getModule() were replaced with
>> getCallerModule(caller) which returns the system class loader unnamed module
>> if the caller is null.
>>
>> The one place a class loader was produced from the caller in getBundleImpl
>> it was rewritten to route through the getCallerModule method:
>>
>> final ClassLoader loader = (caller != null) ?
>> caller.getClassLoader() : getLoader(getCallerModule(caller));
>>
>> A JNI test was added which calls getBundle to fetch a test bundle from a
>> location added to the classpath, fetches a string out of the bundle and
>> verifies it, and calls clearCache.
>>
>> The javadoc was updated for the caller sensitive methods changed.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> suggested changes
Thanks for the update.
src/java.base/share/classes/java/util/ResourceBundle.java line 1567:
> 1565: * module will be used.
> 1566: * @param caller
> 1567: * @return
Maybe you intended to make this `/* */` instead of javadoc? no need to
have `@param` and `@return` for this simple method.
src/java.base/share/classes/java/util/ResourceBundle.java line 2251:
> 2249: @CallerSensitive
> 2250: public static final void clearCache() {
> 2251: final Module callerModule =
> getCallerModule(Reflection.getCallerClass());
nit: final can be dropped here.
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
line 52:
> 50: import java.nio.file.Paths;
> 51: import java.util.Properties;
> 52: import java.util.ResourceBundle;
nit: good to keep the imports in alphabetic order.
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
line 74:
> 72:
> 73: // set up shared library path
> 74: final String sharedLibraryPathEnvName =
> Platform.sharedLibraryPathVariableName();
Nit: `final` adds noise but no other value to the test. Can consider dropping
it.
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
line 89:
> 87:
> 88:
> 89: private static void makePropertiesFile() throws IOException {
This can be removed?
test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/exeNullCallerResourceBundle.c
line 28:
> 26:
> 27: #include "jni.h"
> 28: #undef NDEBUG
is this line unintended?
-
PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]
> The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: suggested changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7663/files - new: https://git.openjdk.java.net/jdk/pull/7663/files/9e8fb193..eeb2d0fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7663&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7663&range=01-02 Stats: 48 lines in 1 file changed: 6 ins; 41 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v2]
> The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/util/ResourceBundle.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.java.net/jdk/pull/7663/files - new: https://git.openjdk.java.net/jdk/pull/7663/files/30778063..9e8fb193 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7663&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7663&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
On Wed, 2 Mar 2022 18:56:40 GMT, Tim Prinzing wrote:
> The caller class returned by Reflection::getCallerClass was used to gain
> access to it's module in most cases and class loader in one case. I added a
> method to translate the caller class to caller module so that the decision of
> what module represents the caller with no stack frame is made in a single
> place. Calls made to caller.getModule() were replaced with
> getCallerModule(caller) which returns the system class loader unnamed module
> if the caller is null.
>
> The one place a class loader was produced from the caller in getBundleImpl it
> was rewritten to route through the getCallerModule method:
>
> final ClassLoader loader = (caller != null) ?
> caller.getClassLoader() : getLoader(getCallerModule(caller));
>
> A JNI test was added which calls getBundle to fetch a test bundle from a
> location added to the classpath, fetches a string out of the bundle and
> verifies it, and calls clearCache.
>
> The javadoc was updated for the caller sensitive methods changed.
Instead of sprinkling the null caller text in the javadoc in each `getBundle`
method, we can specify that that in the class specification, for example, after
the "Resource bundles in other modules and class path" section.
In cases where the {@code getBundle} factory method is called from a context
where there is no caller frame on the stack (e.g. when called directly from
a JNI attached thread), the caller module is default to the unnamed module for
the
{@linkplain ClassLoader#getSystemClassLoader system class loader}.
What do you think?
src/java.base/share/classes/java/util/ResourceBundle.java line 1588:
> 1586: Control control) {
> 1587: final ClassLoader loader = (caller != null) ?
> 1588: caller.getClassLoader() :
> getLoader(getCallerModule(caller));
Suggestion:
ClassLoader loader = getLoader(getCallerModule(caller));
This can be simplified as above. I would drop `final` too as it only adds
noise to the code.
-
PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
On Wed, 2 Mar 2022 18:56:40 GMT, Tim Prinzing wrote: > The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Looks good to me. I believe a CSR is needed for this change. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7663
RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
The caller class returned by Reflection::getCallerClass was used to gain access to it's module in most cases and class loader in one case. I added a method to translate the caller class to caller module so that the decision of what module represents the caller with no stack frame is made in a single place. Calls made to caller.getModule() were replaced with getCallerModule(caller) which returns the system class loader unnamed module if the caller is null. The one place a class loader was produced from the caller in getBundleImpl it was rewritten to route through the getCallerModule method: final ClassLoader loader = (caller != null) ? caller.getClassLoader() : getLoader(getCallerModule(caller)); A JNI test was added which calls getBundle to fetch a test bundle from a location added to the classpath, fetches a string out of the bundle and verifies it, and calls clearCache. The javadoc was updated for the caller sensitive methods changed. - Commit messages: - Use the unnamed module defined to the system class loader instead of the - JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame Changes: https://git.openjdk.java.net/jdk/pull/7663/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7663&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280902 Stats: 265 lines in 4 files changed: 254 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: 8282507: Add LICENSE file for hsdis
On Tue, 1 Mar 2022 20:18:11 GMT, Man Cao wrote: > Hi all, > > Could anyone help review the addition of LICENSE file to hsdis directory? > > -Man Thank you for reaching out for confirmation. This doc might help: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/licensing-a-repository . A separate LICENSE file is also more consistent with other code in OpenJDK. - PR: https://git.openjdk.java.net/jdk/pull/7649
Re: RFR: 8282567: Improve source-date handling in build system
On Wed, 2 Mar 2022 16:52:12 GMT, Magnus Ihse Bursie wrote: > When doing reproducible builds, controlling the build time is imperative. To > make this as easy as possible, some changes are needed in the build system. > > * If source-date is set to anything other than "updated" (meaning that it > should be determined at build time), then the default value of > --with-hotspot-build-time should be the same time. > > * If the industry standard SOURCE_DATE_EPOCH is set when running configure, > the default value for --with-source-date should be picked up from this > variable. (If the command line option is given, it will override the > environment variable however.) > > * Finally the code can be made a bit clearer that we can set and export > SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the > "updated" (determined at build time) value. I think building.md needs to be updated to reflect these changes, as you no longer need to manually propagate the value of SOURCE_DATE_EPOCH through --with-source-date as the documentation currently suggests. - PR: https://git.openjdk.java.net/jdk/pull/7660
Integrated: 8209784: Include hsdis in the JDK
On Tue, 22 Feb 2022 16:10:22 GMT, Magnus Ihse Bursie wrote: > This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting > this, together with a valid backend using `--with-hsdis`, will bundle the > generated hsdis library with the JDK. > > The PR also includes a refactoring of the hsdis handling in autoconf, > breaking it out to a separate file `lib-hsdis.gmk`, and breaking the > functionality up in separate functions per backend. This pull request has now been integrated. Changeset: b6c35ae4 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/b6c35ae44aeb31deb7a15ee2939156ed00b3df52 Stats: 676 lines in 8 files changed: 386 ins; 276 del; 14 mod 8209784: Include hsdis in the JDK Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/7578
Re: RFR: 8282567: Improve source-date handling in build system
On Wed, 2 Mar 2022 16:54:21 GMT, Magnus Ihse Bursie wrote: > To clarify, the end effect of these changes is that building OpenJDK will > basically be compliant with the method of just setting SOURCE_DATE_EPOCH. > (The caveat is that it must be set at configure time, not build time.) So > > ``` > $ export SOURCE_DATE_EPOCH=123 > $ bash configure > $ make > ``` Do you have an example configure output when used like that post-patch? > will cause the build to default to building in reproducible mode, with the > date given by SOURCE_DATE_EPOCH. Hmm, when building via RPM, `SOURCE_DATE_EPOCH` is usually set. If I understand this correctly, prior to this, the RPM build would be non-reproducible. After it it would be. That may be confusing to some. If post-patch this makes the build reproducible, does it mention that via a WARNING/INFO or anything like that after this patch? I don't see it. Am I missing it? It would be good to have something to that effect in the configure output. - PR: https://git.openjdk.java.net/jdk/pull/7660
Re: RFR: 8209784: Include hsdis in the JDK [v3]
On Wed, 2 Mar 2022 14:30:43 GMT, Magnus Ihse Bursie wrote: >> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting >> this, together with a valid backend using `--with-hsdis`, will bundle the >> generated hsdis library with the JDK. >> >> The PR also includes a refactoring of the hsdis handling in autoconf, >> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the >> functionality up in separate functions per backend. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > install-hsdis needs to depend on jdk-image Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7578
Re: RFR: 8282567: Improve source-date handling in build system
On Wed, 2 Mar 2022 16:52:12 GMT, Magnus Ihse Bursie wrote: > When doing reproducible builds, controlling the build time is imperative. To > make this as easy as possible, some changes are needed in the build system. > > * If source-date is set to anything other than "updated" (meaning that it > should be determined at build time), then the default value of > --with-hotspot-build-time should be the same time. > > * If the industry standard SOURCE_DATE_EPOCH is set when running configure, > the default value for --with-source-date should be picked up from this > variable. (If the command line option is given, it will override the > environment variable however.) > > * Finally the code can be made a bit clearer that we can set and export > SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the > "updated" (determined at build time) value. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7660
RFR: 8282567: Improve source-date handling in build system
When doing reproducible builds, controlling the build time is imperative. To make this as easy as possible, some changes are needed in the build system. * If source-date is set to anything other than "updated" (meaning that it should be determined at build time), then the default value of --with-hotspot-build-time should be the same time. * If the industry standard SOURCE_DATE_EPOCH is set when running configure, the default value for --with-source-date should be picked up from this variable. (If the command line option is given, it will override the environment variable however.) * Finally the code can be made a bit clearer that we can set and export SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the "updated" (determined at build time) value. - Commit messages: - Allow setting source date using SOURCE_DATE_EPOCH to configure - Fix else nesting - --source-date improvements Changes: https://git.openjdk.java.net/jdk/pull/7660/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7660&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282567 Stats: 67 lines in 5 files changed: 48 ins; 2 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/7660.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7660/head:pull/7660 PR: https://git.openjdk.java.net/jdk/pull/7660
Re: RFR: 8282567: Improve source-date handling in build system
On Wed, 2 Mar 2022 16:52:12 GMT, Magnus Ihse Bursie wrote: > When doing reproducible builds, controlling the build time is imperative. To > make this as easy as possible, some changes are needed in the build system. > > * If source-date is set to anything other than "updated" (meaning that it > should be determined at build time), then the default value of > --with-hotspot-build-time should be the same time. > > * If the industry standard SOURCE_DATE_EPOCH is set when running configure, > the default value for --with-source-date should be picked up from this > variable. (If the command line option is given, it will override the > environment variable however.) > > * Finally the code can be made a bit clearer that we can set and export > SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the > "updated" (determined at build time) value. To clarify, the end effect of these changes is that building OpenJDK will basically be compliant with the method of just setting SOURCE_DATE_EPOCH. (The caveat is that it must be set at configure time, not build time.) So $ export SOURCE_DATE_EPOCH=123 $ bash configure $ make will cause the build to default to building in reproducible mode, with the date given by SOURCE_DATE_EPOCH. - PR: https://git.openjdk.java.net/jdk/pull/7660
Re: RFR: 8209784: Include hsdis in the JDK [v2]
On Wed, 2 Mar 2022 13:12:52 GMT, Magnus Ihse Bursie wrote: >> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting >> this, together with a valid backend using `--with-hsdis`, will bundle the >> generated hsdis library with the JDK. >> >> The PR also includes a refactoring of the hsdis handling in autoconf, >> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the >> functionality up in separate functions per backend. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Somehow the tabs got converted to spaces Top level install-hsdis needs to depend on jdk-image. - Changes requested by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7578
Re: RFR: 8209784: Include hsdis in the JDK [v2]
> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting > this, together with a valid backend using `--with-hsdis`, will bundle the > generated hsdis library with the JDK. > > The PR also includes a refactoring of the hsdis handling in autoconf, > breaking it out to a separate file `lib-hsdis.gmk`, and breaking the > functionality up in separate functions per backend. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Somehow the tabs got converted to spaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/7578/files - new: https://git.openjdk.java.net/jdk/pull/7578/files/0f19696e..6839f911 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7578&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7578&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7578/head:pull/7578 PR: https://git.openjdk.java.net/jdk/pull/7578
Re: JDK-8282532
Hi Julian, Thanks for bringing this back in a discussion format. If I understand your problem correctly, it is that the --openjdk-target fully replaces the autoconf --build/--host/--target triplet, and assumes that it can autodetect the build platform. And if that autodetect fails, the only resort is to use the full autoconf triplet. The problem with the autoconf triplet is with how they use "host" and "target". The "build" part is the same as we use it, namely the platform used to create the build. So if we want to override the build platform, it should be as simple as setting the --build option to autoconf. That means that the configure wrapper will need to detect if --build is set, and only use our own autodetect if it is not, and change the test for conflicting options to only check host/target at the same time as --openjdk-target. Finally documentation and error messages in the configure wrapper needs to be adapted. Also, the order of configure command options do not matter. /Magnus On 2022-03-02 13:28, Jules W. wrote: Apologies for not creating a thread consulting the mailing list before submitting the PR, I'm still getting used to the process (And also thought this would create unnecessary noise for people within this list). I initially believed that given how the legacy options had warnings when they were set, and how they were discouraged in many areas, that they were eventually to be removed, but that assumption I made appears to be wrong. I was more concerned about how easy it was to understand what the option did than the name conformation, --current-platform was the best one i could come up with so far; --openjdk-build sounded slightly less clear to me when i was coming up with the name. That aside, I'm not sure if keeping the old autoconf --build option is possible, since it seems that --openjdk-target formats --build, --host and --target in a specific order, and whether using the existing --build option would affect it in any way, since that would mean --build is now behind --host and --target (Given how --openjdk-target works, from what I can tell). I haven't changed the html version of the documentation yet as I haven't really finished making changes to the markdown version, I do apologize for the formatting errors though. best regards, Julian
Re: JDK-8282532
Hi Thomas, I was referring to Magnus' issue with the markdown docs I edited when I submitted the PR. Thank you for the encouragement, it means a lot to me :) best regards, Julian On Wed, Mar 2, 2022 at 8:42 PM Thomas Stüfe wrote: > Hi Jules, > > On Wed, Mar 2, 2022 at 1:29 PM Jules W. wrote: > >> Apologies for not creating a thread consulting the mailing list before >> submitting the PR, I'm still getting used to the process (And also thought >> this would create unnecessary noise for people within this list). >> > > On the contrary, this is very much the encouraged behavior. It's never > wrong to ask around before doing a patch. Someone may already be doing it, > or people have different notions about how to solve the problem. Or whether > it is a problem to begin with. > > And if you don't get answers, nobody cares enough and it is okay to > proceed with a PR. > > >> >> I haven't changed the html version of the documentation yet as I haven't >> really finished making changes to the markdown version, I do apologize >> for > > the formatting errors though. >> >> > Do you refer to something you attached? Note that attachments are scraped > from mails in the ML. > > Cheers, Thomas >
Re: JDK-8282532
Hi Jules, On Wed, Mar 2, 2022 at 1:29 PM Jules W. wrote: > Apologies for not creating a thread consulting the mailing list before > submitting the PR, I'm still getting used to the process (And also thought > this would create unnecessary noise for people within this list). > On the contrary, this is very much the encouraged behavior. It's never wrong to ask around before doing a patch. Someone may already be doing it, or people have different notions about how to solve the problem. Or whether it is a problem to begin with. And if you don't get answers, nobody cares enough and it is okay to proceed with a PR. > > I haven't changed the html version of the documentation yet as I haven't > really finished making changes to the markdown version, I do apologize for the formatting errors though. > > Do you refer to something you attached? Note that attachments are scraped from mails in the ML. Cheers, Thomas
JDK-8282532
Apologies for not creating a thread consulting the mailing list before submitting the PR, I'm still getting used to the process (And also thought this would create unnecessary noise for people within this list). I initially believed that given how the legacy options had warnings when they were set, and how they were discouraged in many areas, that they were eventually to be removed, but that assumption I made appears to be wrong. I was more concerned about how easy it was to understand what the option did than the name conformation, --current-platform was the best one i could come up with so far; --openjdk-build sounded slightly less clear to me when i was coming up with the name. That aside, I'm not sure if keeping the old autoconf --build option is possible, since it seems that --openjdk-target formats --build, --host and --target in a specific order, and whether using the existing --build option would affect it in any way, since that would mean --build is now behind --host and --target (Given how --openjdk-target works, from what I can tell). I haven't changed the html version of the documentation yet as I haven't really finished making changes to the markdown version, I do apologize for the formatting errors though. best regards, Julian
Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags
On Wed, 2 Mar 2022 08:11:51 GMT, TheShermanTanker wrote: > Currently the only other option for manually configuring the build platform > while cross compiling are devkits, which don't work on certain systems and > are also more focused on differentiating the build and target compilers > instead. This patch adds the ability to explicitly set the build platform > through a new option, which can be especially helpful for when autodetection > fails and devkits cannot be relied on, and also for simpler cross compilation > cases (Like the one described in building.md) > > This also removes support for the legacy cross compilation flags as well. > > WIP: Translation from markdown to html Alright, it seems I may not have selected the best approach towards the issue, marking it as a draft for the time being while I communicate with the mailing list for a better solution - PR: https://git.openjdk.java.net/jdk/pull/7656
Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags
On Wed, 2 Mar 2022 08:11:51 GMT, TheShermanTanker wrote: > Currently the only other option for manually configuring the build platform > while cross compiling are devkits, which don't work on certain systems and > are also more focused on differentiating the build and target compilers > instead. This patch adds the ability to explicitly set the build platform > through a new option, which can be especially helpful for when autodetection > fails and devkits cannot be relied on, and also for simpler cross compilation > cases (Like the one described in building.md) > > This also removes support for the legacy cross compilation flags as well. > > WIP: Translation from markdown to html As Thomas says, it would have been much better if you raised the issue on the build-dev mailing list first. This does not seem to be the right way to resolve your problem. I have several objections to this PR. There is no need to remove support for the traditional autoconf arguments. Sure, it's confusing and does not match our build model, but we don't know who are actually using it out in the wild nevertheless. We have the functionality. Don't remove it. If we are to add a new argument to configure to specify the build platform, it should align with existing terminology. Just as --openjdk-target matches the OPENJDK_TARGET_* variables, so should a new argument match the OPENJDK_BUILD_* variables. That is, it should be named --openjdk-build. That said, I'm not even sure we *need* a new argument. If I understand you correctly, all you want to do is to be able to set --openjdk-target *and* use the traditional autoconf --build at the same time, right? We're currently blocking this since it was assumed users were doing something wrong, but there might be good reasons for doing this. And finally, changes in building.md should reflect the current line length of 80. And any changes needs to be updated into building.html (by running `make update-build-docs` with a proper version of pandoc). But once again, I don't think the current approach is a good one. - Changes requested by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7656
Re: RFR: 8282507: Add LICENSE file for hsdis
On Tue, 1 Mar 2022 20:18:11 GMT, Man Cao wrote: > Hi all, > > Could anyone help review the addition of LICENSE file to hsdis directory? > > -Man I'm not sure what the legal implications are of adding a separate general file like this, instead of marking each individual file. I'd need to get some confirmation that this is acceptable. - PR: https://git.openjdk.java.net/jdk/pull/7649
Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags
On Wed, 2 Mar 2022 08:33:58 GMT, TheShermanTanker wrote: > > > This also removes support for the legacy cross compilation flags as well. > > > > > > Hi, > > without reading through building.md and your patch, which legacy flags are > > would be removed by this? > > Thanks, Thomas > > Specifically the flags that are checked if openjdk-target is specified, in > make/autoconf/configure (Essentially just passing --build, --host and > --target directly to configure, something which is largely discouraged within > the JDK): > > ``` > if test "x$conf_legacy_crosscompile" != "x"; then > if test "x$conf_openjdk_target" != "x"; then > echo "Error: Specifying --openjdk-target together with autoconf" > echo "legacy cross-compilation flags is not supported." > echo "You specified: --openjdk-target=$conf_openjdk_target and > $conf_legacy_crosscompile." > echo "The recommended use is just --openjdk-target." > exit 1 > else > echo "Warning: You are using legacy autoconf cross-compilation flags." > echo "It is recommended that you use --openjdk-target instead." > echo "" > fi > fi > ``` Okay, thank you for explaining. I cannot comment on those since I don't use them. > > > Oh, and please describe whatever you do in the JBS issue. It's completely > > blank. It would be nice if it contained a description of what you do, at > > least as good as the PR text. Thank you! > > Will do, thanks for the heads up! Sure :) For most of us, JBS is the main source of information and archiving, so we try to keep its content high quality. I usually beef out the JBS issue text first and just paste that one verbatim into the PR description for the convenience of reviewers. Did you discuss this first on the build-dev mailing list? I may have missed this discussion. If not, this is often a good first step, especially seeing that you are new. It prevents frustration later, e.g. when you code in the wrong direction. Another small tip is to use draft PRs. Especially with build changes, it makes sense to create the PR as Draft, then wait for the GHAs to complete successfully on all platforms, and only then un-draft the PR. (e.g. with your patch, I see already failures on 32-bit). That prevents MLs being flooded by skara for work which is still in progress. Cheers, and welcome to the OpenJDK! ..Thomas - PR: https://git.openjdk.java.net/jdk/pull/7656
Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags
On Wed, 2 Mar 2022 08:27:19 GMT, Thomas Stuefe wrote: > > This also removes support for the legacy cross compilation flags as well. > > Hi, > > without reading through building.md and your patch, which legacy flags are > would be removed by this? > > Thanks, Thomas Specifically the flags that are checked if openjdk-target is specified, in make/autoconf/configure (Essentially just passing --build, --host and --target directly to configure, something which is largely discouraged within the JDK): if test "x$conf_legacy_crosscompile" != "x"; then if test "x$conf_openjdk_target" != "x"; then echo "Error: Specifying --openjdk-target together with autoconf" echo "legacy cross-compilation flags is not supported." echo "You specified: --openjdk-target=$conf_openjdk_target and $conf_legacy_crosscompile." echo "The recommended use is just --openjdk-target." exit 1 else echo "Warning: You are using legacy autoconf cross-compilation flags." echo "It is recommended that you use --openjdk-target instead." echo "" fi fi > Oh, and please describe whatever you do in the JBS issue. It's completely > blank. It would be nice if it contained a description of what you do, at > least as good as the PR text. Thank you! Will do, thanks for the heads up! - PR: https://git.openjdk.java.net/jdk/pull/7656
Re: RFR: JDK-8282532: Add option to explicitly set build platform and remove support for legacy cross compilation flags
On Wed, 2 Mar 2022 08:11:51 GMT, TheShermanTanker wrote: >This also removes support for the legacy cross compilation flags as well. Hi, without reading through building.md and your patch, which legacy flags are would be removed by this? Thanks, Thomas Oh, and please describe whatever you do in the JBS issue. It's completely blank. It would be nice if it contained a description of what you do, at least as good as the PR text. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/7656
RFR: JDK-8282532 Add option to explicitly set build platform and remove support for legacy cross compilation flags
Currently the only other option for manually configuring the build platform while cross compiling are devkits, which don't work on certain systems and are also more focused on differentiating the build and target compilers instead. This patch adds the ability to explicitly set the build platform through a new option, which can be especially helpful for when autodetection fails and devkits cannot be relied on, and also for simpler cross compilation cases (Like the one described in building.md) This also removes support for the legacy cross compilation flags as well. WIP: Translation from markdown to html - Commit messages: - Make error message clearer - Allow explicit setting of the build platform when cross compiling Changes: https://git.openjdk.java.net/jdk/pull/7656/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7656&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282532 Stats: 40 lines in 2 files changed: 21 ins; 3 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/7656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7656/head:pull/7656 PR: https://git.openjdk.java.net/jdk/pull/7656
