Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove java.security.manager=allow flag from windows launcher. #8009

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mbien
Copy link
Member

@mbien mbien commented Nov 29, 2024

this commit implements SM removal step 1 of 3

  1. remove SM flag from windows launcher and release new launcher bits
  2. move flag to config and switch to new launcher bits
  3. implement SM removal for JDK 24 compatibility

extracted from #7928

this changes it only in the windows launcher but keeps it in the bash script. I think we should change it in the linux launcher when we move it into the config and switch to new windows launcher bits in step 2) but not here.

jargs_without_clusters="$jargs -Djava.security.manager=allow"

this essentially reverts apache/netbeans-native-launchers@3fea281 although the code is in a different repo now.

@mbien mbien added the Platform [ci] enable platform tests (platform/*) label Nov 29, 2024
@mbien mbien added this to the NB25 milestone Nov 29, 2024
@mbien mbien force-pushed the remove-sm-flag-from-launcher branch from d73956b to c9ef14a Compare November 29, 2024 23:52
@mbien mbien requested a review from neilcsmith-net November 29, 2024 23:55
@lkishalmi
Copy link
Contributor

Not familiar with the windows native launcher though it looks good to me.

@mbien
Copy link
Member Author

mbien commented Dec 2, 2024

Not familiar with the windows native launcher though it looks good to me.

me neither. I have to check the past PRs and take a look what needs to be done before a release. Probably a version bump somewhere too. Then actually test it on windows.

@neilcsmith-net
Copy link
Member

@mbien well, it should be testable from https://github.com/apache/netbeans/actions/runs/12091765448

Will need a version update before attempting to move to vote, in manner outlined at #6869 (comment)

There are a couple of other changes that would be nice to get in to the launcher if we're going for an update, but something to discuss elsewhere ...

@mbien mbien force-pushed the remove-sm-flag-from-launcher branch from c9ef14a to 9f80a3c Compare December 4, 2024 08:51
@mbien mbien changed the title Remove java.security.manager=allow flag from launcher. Remove java.security.manager=allow flag from windows launcher. Dec 4, 2024
@mbien mbien force-pushed the remove-sm-flag-from-launcher branch from 9f80a3c to 0130e21 Compare December 4, 2024 09:01
@neilcsmith-net
Copy link
Member

neilcsmith-net commented Dec 4, 2024

@mbien check b8e969a which has all the places this needs setting.

Please use 101.2.0.0 - unless there's a particular reason not to? Build ID wrapping, possibly? At least, the idea was to use strictly increment numbers and not suggest semantic versioning in any way.

Please do the version change in a separate commit this time - I failed to do that, but should have in hindsight.

@mbien mbien force-pushed the remove-sm-flag-from-launcher branch 2 times, most recently from cece6df to bbfed41 Compare December 4, 2024 09:26
@mbien mbien force-pushed the remove-sm-flag-from-launcher branch from bbfed41 to a4c5608 Compare December 15, 2024 13:12
@mbien
Copy link
Member Author

mbien commented Dec 15, 2024

the launcher build started failing recently.

 jvmlauncher.cpp: In member function ‘bool JvmLauncher::startInProcJvm(const char*, const std::__cxx11::list<std::__cxx11::basic_string<char> >&, const std::__cxx11::list<std::__cxx11::basic_string<char> >&)::Jvm::init(const std::__cxx11::list<std::__cxx11::basic_string<char> >&)’:
jvmlauncher.cpp:252:61: error: ordered comparison of pointer with integer zero (‘HMODULE’ {aka ‘HINSTANCE__*’} and ‘int’)
  252 |                 if (option.find("-splash:") == 0 && hSplash > 0) {
      |                                                     ~~~~~~~~^~~
make: *** [Makefile.mingw:34: nbexec64.dll] Error 1

Probably due to the ubuntu image update. I take a look if i can fix the pointer comparison.

@mbien mbien force-pushed the remove-sm-flag-from-launcher branch 2 times, most recently from 4a268de to 3389620 Compare December 15, 2024 17:06
@mbien
Copy link
Member Author

mbien commented Dec 15, 2024

i tested this with the following setup:

and it seems to work fine:

-------------------------------------------------------------------------------
>Log Session: Sunday, December 15, 2024, 5:22:23 PM Greenwich Mean Time
>System Info: 
 Product Version         = Apache NetBeans IDE DEV (Build dev-3687560331e398c3854f24675c36c44c48dcb804)
 Operating System        = Windows 10 version 10.0 running on amd64
 Java; VM; Vendor        = 24-ea; OpenJDK 64-Bit Server VM 24-ea+28-3562; Oracle Corporation
 Runtime                 = OpenJDK Runtime Environment 24-ea+28-3562
 Java Home               = C:\Users\virtual10\Downloads\openjdk-24-ea+28_windows-x64_bin\jdk-24

@neilcsmith-net
Copy link
Member

The need to adapt to the compiler on the newer CI image concerns me. I think we should pin all the native build workflows to the lowest supported image for broadest compatibility, before moving to further releases of any. Ideally we'd disconnect the runner OS from the compiler and library versions.

@mbien
Copy link
Member Author

mbien commented Dec 16, 2024

The need to adapt to the compiler on the newer CI image concerns me.

the NULL comparison change is something which should be done in any case. Comparing a pointer using > with 0 seems to be error prone - a warning might have switched to an error.

@neilcsmith-net
Copy link
Member

The change itself is fine, the need to change, which shows that the build environment we're using for the distributed natives has changed, is what concerns me.

this commit implements SM removal step 1 of 3

 1) remove SM flag from win launcher and release new launcher bits
 2) move flag to config and switch to new launcher bits
 3) implement SM removal for JDK 24 compatibility
@mbien mbien force-pushed the remove-sm-flag-from-launcher branch from 3389620 to a2d9c1a Compare December 16, 2024 15:42
@mbien
Copy link
Member Author

mbien commented Dec 16, 2024

rebased on top of #8062 which locks binaries build to ubuntu 22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os:windows Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants