On Mon, 8 Jul 2024 16:40:50 GMT, Andrew Haley <a...@openjdk.org> wrote:
>> Hamlin Li has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains 33 commits: >> >> - Merge branch 'master' into sleef-aarch64-integrate-source >> - merge master >> - sleef 3.6.1 for riscv >> - sleef 3.6.1 >> - update header files for arm >> - add inline header file for riscv64 >> - remove notes about sleef changes >> - fix performance issue >> - disable unused-function warnings; add log msg >> - minor >> - ... and 23 more: https://git.openjdk.org/jdk/compare/2f4f6cc3...b54fc863 > >> While I agree with you in principle, we chose to import Sleef this way for >> practical reasons. (The actual importing of Sleef is happening in #19185 / >> [JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816).) The >> "preprocessing/code-generation" part of the Sleef build was considered too >> complex to reasonably replicate in the OpenJDK build system. Sleef is built >> using Cmake and we do not want to add a build dependency on Cmake and call >> out to a foreign build system at build time, for efficiency and complexity >> reasons. > > Of course, there is no reason to rebuild the preprocessed headers every time > we build the JDK. I'd never ask for that; the last thing I want is to make > building the JDK slower. However, it should be possible to do so on a > checked-out JDK source tree, at the builder's option. > > If there is a script, it doesn't have to be included in the OpenJDK build > system itself, but it does have to be in the OpenJDK source tree. (It could > be part of make/devkit, for example.) > > With a script to produce preprocessed files, it should be possible for anyone > building the JDK to run that script, and produce the preprocessed source. > SLEEF won't take up a prohibitive amount of space. > > We shouldn't be depending on some other web site somewhere being able to come > up with the exact SLEEF sources we used, either. That fails the test of > reproducibility. > >> JDK-8329816 comes with a script to automatically generate the imported >> source files, to make it easy to update Sleef in the future. It should also >> be easy enough to verify the imported contents using the same script for >> anyone who wants to check the validity of the import step. > > I get it, but not including everything we use in the OpenJDK tree is a > dangerous precedent. It should be no big deal to do this right, given that we > have the SLEEF sources and the build scripts already. I'm not asking for > anything that doesn't exist already, I'm just saying that it must be checked > in. > > Avoiding inconvenience, however great, is not sufficient to justify such a > step. This is perhaps something to discuss at the next Committers' Workshop. @theRealAph a precendent that exists is for binutils/llvm/capstone and hsdis. Would it be sufficient for the user to choose to build SLEEF from a separate source directory assuming all the dependencies are installed already (the source are checked-out by the user; cmake and other build dependencies are installed; etc.)? We would then invoke the [make/devkit/createSleef.sh](https://github.com/openjdk/jdk/pull/19185/files#diff-4fe89562540474e866588cd87ca7385b920a06bd428da013cd3d3e4b375fdd10) script on the user's SLEEF checkout to regenerate the header files. And by default, we use the header files already checked-in the OpenJDK. ------------- PR Comment: https://git.openjdk.org/jdk/pull/18605#issuecomment-2219783035
