[NETBEANS-1428] Fix for profiler crash - with CI/CD workflow #2700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Stacks.c: corrected some incorrect casts for 64-bit windows. Implemented mapping from methodIds to ints using a hash table for 64-bit platforms. * Updated build scripts for Windows, Linux and Mac. * Moved scripts out of the build subdirectory.
Build scripts for legacy platforms moved to 'legacy' folder to signal that these scripts are no longer being maintained .. nor used. As of now we focus on the following platforms only: Linux X64 (64-bit) Linux X86 (32-bit) Windows X64 (64-bit) Windows X86 (32-bit) MacOS X64 (64-bit) Additonal note: Generating JNI headers for Java classes is now part of the Ant build so the 'generate-headers-15.bat' file is also no longer used.
Windows only: This information shows up if you right-click the DLL in Windows File Explorer and choose 'Properties' --> 'Details'. Properly very few people would do that. All information here, including the version information, is irrelevant. The version number is just an arbitrary string. There is no ambition that this version number follows the general NetBeans versioning.
Since the build scripts will from now on be used in a CI/CD pipeline they need to be strictly correct, not have bugs and not require manual intervention. Therefore... Fixes: - The Windows build scripts did not incorporate Peter's fix, i.e. creating the 'config.h' file as a pre-cursor to the actual build. This is now fixed. (compare to build scripts for MacOS and Linux) - The Windows build script did not include headers from '..\build' when compiling. They therefore did not work. This is now fixed. (this is a consequence of the JNI headers having been removed from source control in commit f395ace; the change was correctly implemented at the time in build scripts for Linux and MacOS but for some reason never in build scripts for Windows) - The Windows build scripts will now stop on first error. (checks for exit code and abends immediately). The Linux and MacOS scripts do not need the same logic as such script can simply be executed with 'bash -x <script>'. - The content of the 'config.h' file is now echoed to the console as part of the build. This file is crucial to understand the fix for the bug reported in NETBEANS-1428. - Since we will be creating 32-bit binaries on 64-bit platforms (because we can no longer find 32-bit hosts to build on): The Linux 32-bit script did not create a 32-bit binary for the 'config' application, hence it produced an incorrect 'config.h' file. - The Windows build scripts could not handle path names with spaces in them as is typical in Windows world. Hence the script did not work when executed from a GitHub Action workflow. This is now fixed. (too lazy to fix the same problem in the MacOS and Linux scripts; on those platforms the problem never manifests itself) - MacOS script: was hardwired to use the installed JDK rather than using the JDK_HOME environment variable. This is now fixed. - MacOS script: added comments and transparency about how the Universal Library mechanism works. (the result of the mechanism is that binaries for legacy platforms PowerPC and i386 architectures still exist in the bundle but do not receive bug fixes .. meaning they'll not as an example receive bug fix for NETBEANS-1428) - The Windows build scripts used Unix line endings. Although Windows doesn't mind this when executing such script it has now been corrected.
|
A cause for celebration.
So should the internals change in the future, the manual step is required to upload?
It it correct that these other platforms will continue to work (assuming the internals aren't changed)? Can you get any reviewers assigned? |
|
Big thumbs up @lbruun, I'm very glad you've been able to finish this off properly. |
|
…filer The workflow builds 'profilerinterface' library on the currently maintained platforms (see file header) and produces a single ZIP file which is suitable for upload to https://netbeans.osuosl.org/binaries/.
|
Merging as per the discussion on the dev mailing list. |
|
This is great. Thanks @lbruun ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for [NETBEANS-1428]
This PR replaces #2021.
This PR builds upon the fixes made by Peter Hull (I've cherry-picked commits from PR-2021) but corrects a number of issues, changes copyright from Oracle to ASF and finally adds a GitHub Actions workflow which builds the native binary on Windows, Linux and MacOS in a manner which is consistent, transparent and reproducible. The core component of the fix is still Peter's. Peter analyzed the bug, diagnosed it and came up with a solution (partly based on discussion on the OpenJDK mailing list) so all credit to Peter!!.
Background
The reason for the bug is that the original C code in lib.profiler, which acts as JNI interface, made an incorrect assumption about how to convert from
jinttojmethodIDand in doing so effectively relied on an undocumented JVM internal. This assumption was broken with Java 9 on Windows, hence the crash. Peter's fix is to have a HashMap in the C code which can translate fromjinttojmethodIDbut only do so on platforms where the two are of different implementation and therefore need conversion.It can be said that the bug existed on all platforms, but because of some JVM internals only manifests itself on the Windows platform. Peter's fix targets all platforms and this is important as we are then immune to the bug if the JVM in future changes its (undocumented) internals for Linux or MacOS. Therefore it is important to re-build binaries not only for Windows, but for other platforms too.
Produced artifacts
The GitHub Workflow added in this PR produce binaries for
as all other platforms are now considered unsupported from NetBeans Profiler point of view.
The GitHub workflow produces a ZIP file,
profiler-external-binaries-ASF.zipwhich can be downloaded from the "Artifacts" section of a particular run in the GitHub Actions UI. This ZIP is suitable for upload to https://netbeans.osuosl.org/binaries/.Why another PR?
Although the base of the fix in #2021, the C code, was/is fully correct, I felt there were too many changes required to make the PR work (reproducible build, rather than as a one-time lucky punch) and only the PR creator would have permission to make those changes. It could potentially take many ping-pong discussions. Also this PR has a clear view of how the fix can be incorporated into the NetBeans distribution following acceptance of the PR. See below.
Testing this PR
or rather : testing the new binaries which are a result of this PR.
Provoke the crash
Replace with new binaries
profiler-external-binaries-ASF.zipfrom the "Artifacts" sections of the GitHub Actions UI and explode it onto your NetBeans IDE installation directory, foldernetbeans/profiler/, so that files from the ZIP overwrite what you already have on your harddisk. (you should make a backup prior to this of the files that will be overwritten).For MacOS and Linux there's no crash, so the test should be that the new binaries (also) work.
Next steps
This is essentially a two-step process which requires not one, but two, PRs. This is the first of those PRs.
Once this PR is accepted the GitHub workflow will run and produce artifact. Someone with proper permission must then upload this to
https://netbeans.osuosl.org/binaries/. (naming the file
profiler-external-binaries-ASF.zip)I'll then produce a follow-up PR which incorporates the new ZIP in the Ant build scripts like this;
Currently the Ant build script looks like this:
in the future it will look like this:
In other words: We retain all esoteric binaries from the CDDL licensed bundle but those are overlayed with ones created in ASF (meaning those that are AL2 licensed). The new ASF bundle only include those platforms listed at the top and which are probably the only platforms we can truly support going forward. So for the Windows, MacOS and Linux platforms it will be the binaries from the ASF build which are effectively being used ... and since these are the only ones which has been rebuild it is also the only ones which includes the bug fix for [NETBEANS-1428] but I think that is acceptable as we have no knowledge that the bug will manifest itself on those esoteric platforms. Nor do we have any means of re-building for those platforms. So we leave them in the dust.