Download launcher binaries as produced by #8756

[jtulach]

• Post merge actions of Make sure netbeans in embedded terminal means itself #8756
• Uses the netbeans.exe and netbeans64.exe binaries produced by the merge commit 8e2bc77 github action
  • right now the downloading happens from dev area
  • the bits have been moved to release area as the vote has passed
• wait for CI build to finish
  • do testing on Windows - local build works fine with Cygwin
  • call for a vote like in this case - the vote has passed
  • release the bits to release area and download area
  • update the URL to archive area done in ee5e0f9
  • get the CI green
  • merged as c0059cf

[jtulach]

@matthiasblaesing has discovered ideopen functionality after #8756 was merged. I believe existence of such a functionality doesn't make #8756 obsolete and thus I am proceeding as agreed to. I'd like to:

• make sure terminal integration on Windows works without cygwin
• be able to open files in any terminal like cmd or Windows powershell, not just cygwin
• being able to use other CLI arguments (--close-group, --open-group, LSP options, etc.)

Those additional goals can be achieved most easily by making sure netbeans means NetBeans in the terminal. Thus I'd like to proceed with the originally agreed plan.

[mbien]

after #8756 was merged. I believe existence of such a functionality doesn't make #8756 obsolete and thus I am proceeding as agreed to

I think its worth pointing out that the PR was merged without approval which would usually indicate integration readiness or some kind of consensus between the participating reviewers.

I still think it would have been good to explore some (pure java) init-profile based solutions which are common for situations like this. As mentioned before, they are the standard way how to initialize a terminal and more flexible without having to modify the user's PATH variable.

$ cat nb_profile 
echo "Welcome to the NB Terminal"
alias nb='echo blabla...'

$ bash --init-file <(cat /etc/profile ~/.bashrc nb_profile)
Welcome to the NB Terminal
$ nb
blabla...

More flexibility provided by profiles might be needed, since users will soon ask why does javac not point to the default JDK and mvn doesn't run the bundled maven etc. If PATH concatenation is needed after all, the nb_profile file would be able to do that too as opt-in and it would be fully transparent and configurable by the user.

While I don't like how this was handled I don't care enough to veto this, so I leave it at -0.9 and won't participate in the maintenance of this feature. Removing myself from the reviewers list.

[matthiasblaesing]

I think its worth pointing out that the PR was merged without approval which would usually indicate integration readiness or some kind of consensus between the participating reviewers.

While I think a simpler solution would be possible, we are not in pre-merge approval mode.

[mbien]

While I think a simpler solution would be possible, we are not in pre-merge approval mode.

I didn't try to say that we would be. (We would have enforced it via github pre-merge checks otherwise) I have merged trivial changes before too without review to avoid notification noise. But I don't think a change which requires platform dependent testing and a launcher release would fall into that category.

[jtulach]

change which requires platform dependent testing and a launcher release

• exactly because of the platform dependency there was a lot of testing done by Matthias and me
  • all bugs were addressed
• moreover it was decided to not hurry with this change for NetBeans 28, but wait for NetBeans 29
  • next three months will give us enough time to test the change deeply
• to test we need to update the windows launchers as this PR does
• I suggest to stick to the agreed plan
  • a plan that nobody objected to by the time I merged Make sure netbeans in embedded terminal means itself #8756
  • let's get the plan done - e.g.:
  • vote on the launchers and integrate this PR
• the sooner we integrate, the sooner we all participate in testing

[jtulach]

The build seems to be green. Is there a .zip file to test? I built one manually myself, but having a .zip built by CI somewhere would increase reproducibility.

[matthiasblaesing]

@jtulach the builds for the native bits are triggered by changes to the corresponding paths:

netbeans/.github/workflows/native-binary-build-launcher.yml

Lines 34 to 48 in d753c90

	push:
	# Triggers the workflow on push or pull request events but only for
	# relevant paths
	paths:
	- .github/workflows/native-binary-build-launcher.y*
	- platform/o.n.bootstrap/launcher/windows/**
	- harness/apisupport.harness/windows-launcher-src/**
	- nb/ide.launcher/windows/**
	pull_request:
	paths:
	- .github/workflows/native-binary-build-launcher.y*
	- platform/o.n.bootstrap/launcher/windows/**
	- harness/apisupport.harness/windows-launcher-src/**
	- nb/ide.launcher/windows/**

You can trigger the build from the "Actions" tab of the repository. Select "NetBeans Native Launcher" and then "Run workflow". You can run from master as that hold the latest changes to the launcher.

Or easier: go to checks page of the original PR https://github.com/apache/netbeans/pull/8756/checks, Select "Make sure netbeans in embedded terminal means itself" and you are redirected to the result of that run: https://github.com/apache/netbeans/actions/runs/18612911215. There are your binaries.

[neilcsmith-net]

Please confirm / ensure we have behaviour compatibility in the platform Unix script before this merges.

While I think a simpler solution would be possible, we are not in pre-merge approval mode.

No, we're in post-merge approval mode! We don't have a formal pre-merge approval mode in line with ASF's definition of review-then-commit. And whether we do or not, additional review, requests for changes or vetoing of the whole are still possible up until this actually makes it into a NetBeans release.

I think that the points raised by @matthiasblaesing and @mbien were not properly considered. But while I'm half inclined to force a revert just to ensure that they are, let's keep them under consideration and get this in for testing purposes.

[jtulach]

Please confirm / ensure we have behaviour compatibility in the platform Unix script before this merges.

• how do you want that to be ensured?
  • I believe we both, me and @matthiasblaesing did a lot of testing
  • there was a deep review of Make sure netbeans in embedded terminal means itself #8756 code
  • I am convinced the Unix scripts and Windows launchers behave the same
• in fact the point of integrating this PR is to
  • make sure Windows launchers in master are on par with the Unix ones
  • e.g. the three steps plan is finally finished

I think that the points raised by @matthiasblaesing

• points raised by Matthias were probably addressed as documented
  • by this comment claiming my changes "look safe enough"
  • and Matthias vote on the binaries this PR is using

...get this in for testing purposes...
...stick to the original plan ... let's get the plan done - e.g.:

• vote on the launchers
• and integrate this PR

the sooner we integrate, the sooner we all participate in testing

Let's get the plan done by merging this PR in. Please approve the integration.

[neilcsmith-net]

Please confirm / ensure we have behaviour compatibility in the platform Unix script before this merges.

* how do you want that to be ensured?
  
  * I am convinced the Unix scripts and Windows launchers behave the same

Ah, I missed you'd updated your original comment and added a commit reference to opt out the platform launcher in #8756 (comment) I was looking for changes in the Unix platform launcher as I've been out of the NB loop for a few weeks. I remain in favour, as in the comment below that, of having the platform and IDE launchers aligned.

But, I'll remove the request for changes here as it seems to be inaccurate. Apologies for that.

I think that the points raised by @matthiasblaesing
* points raised by Matthias were probably addressed as documented

  * by this [comment](https://github.com/apache/netbeans/pull/8756#issuecomment-3452292129) claiming my changes  _"look safe enough"_

That's an interesting way of assuming approval given it also states a preference to fix the existing approach. A view which is shared.

Please approve the integration.

Request for changes removed, but I'm not approving. By my calculation these changes are currently about -2.7 (3x -0.9) on the approval scale.

Let's test, and let's also see why fixing ideopen is not the right approach.

Missed commit in Windows launcher that bypasses in platform.

[mbien]

No, we're in post-merge approval mode! We don't have a formal pre-merge approval mode in line with ASF's definition of review-then-commit.

I don't feel to drag this into off topic but since I got pinged again I suppose i can share that the merge history shows that all noteworthy PRs are reviewed before merge, usually indicated by other committers approving them - there are only few exceptions which probably is ok.

We absolutely have the ability to recheck and revert changes post merge, but it certainly is not the rule and it is something we should try to avoid for our own sanity. (we had a fairly low revert count recently which is great!)

I want you just to imagine for a moment a repo of the size of this one here, with 80 committers and no QA team and post merge approval actually practiced. Pre-release would be a full time job of reverts, and within a year nobody would know anymore what is in the repo.

Luckily, this is not how most PRs are merged. They are peer reviewed before merge. We even have some limited CI checks and a review guide which organically grew over time (changes to the guide were also discussed before they were made) to avoid to repeat merge accidents, have a stable master and try to reduce the amount of followup PRs. (merge accidents specifically are something which can't be reverted without force push since those are usually about how it got merged not that it got merged)

So please, just because each of us have the ability to revert something post-merge, lets not pretend that post-merge approval or review is actually the practiced mode, since it does not reflect reality. Again: most relevant PRs are reviewed, check the merge log.

My suggestion is to simply change the phrasing from "we are running in commit then review mode" to "we can also review/revert post-merge if necessary".

(I won't revert this change - don't worry, NETBEANS_USERDIR=IGNORE is already in my bash profile)

Let us go around and help get good PRs in by reviewing them, instead of repeatedly pointing out that anything can be reverted since when using lawyer speak we theoretically review changes post merge.

[matthiasblaesing]

I will ignore the side discussions and just focus on the contents of this PR. It brings the windows launchers in line with the shell scripts. That alone leads to the fact that this has to be done. The binaries were voted on and vote passed. Time to discuss the changes was when the source changes were committed.

This should not be merged as is however. The launchers are used in three places and these two are missing:

• ./harness/apisupport.harness
• ./platform/o.n.bootstrap

@jtulach please have a look here:

• 2e97532 - that is my integration of the last launcher update
• d338eb9 - that is a fix Neil provided that was missing from the first commit

We should not allow the launcher to be partially updated. This would lead to an even more complex situation.

[neilcsmith-net]

@mbien that's a lot of text to reiterate what we already discussed on Slack! 😄 I almost entirely agree with you. That does not remove us from the implications of ASF protocols and guidelines on code modifications. It would be madness for us to move to formal votes on every code modification. But if we want to keep post-merge vetoes and reverts to a minimum we need committers to merge only when they're confident such a vote would pass.

Let's keep any further discussion on that where it belongs. Let's get this in and test. If anyone wishes to continue review or request changes on this they may.

[jtulach]

The launchers are used in three places and these two are missing:

./harness/apisupport.harness
./platform/o.n.bootstrap

We should not allow the launcher to be partially updated. This would lead to an even more complex situation.

• Thanks for the review.
• Originally I wanted to update only netbeans.exe because behavior of all other launchers is unaffected
• However, as you point out, such a partial update would be confusing
• better to replace all the usage of 3-282bbc032bcd with 4-8e2bc77e581e as done in b3f5315
• now there are no references to 282bbc032bcd anymore:

netbeans$ find . | grep 282bbc032bcd
netbeans$ grep -r 282bbc032bcd *

[matthiasblaesing]

Looks sane to me. I think this is fine to go in.

However I noticed a problem of the shell launcher when testing. If you invoke the launcher with a relative path, that relative path is set to NETBEANS_USERDIR. The moment you change the local directory in the terminal this invalidates the reference as now the relative path resolves to a different directry. The windows launchers use the absolute path, which looks like the right solution.

On Linux invoked as nbbuild/netbeans/bin/netbeans --userdir x I get:

On windows I get (with the new launcher):

Please recheck the script launcher again (as a followup, not here).

[mbien]

@mbien that's a lot of text to reiterate what we already discussed on Slack! 😄

@neilcsmith-net if i would have more time I would have written a shorter reply ;)

I could have said that merged PRs without review may cause revert PRs without review, but this would be the wrong mindset IMO and unnecessarily confrontational. Its more constructive to talk about practices which are there to avoid reverts in the first place - even though it requires more text. (and I did also at no point claim that merges would require review)

Sorry to have derailed this PR after getting pinged, but I simply don't think its beneficial to point out that this repo is run in "in post-merge approval mode! ", while it is easily verifiable for anyone that in fact all noteworthy/risky PRs do get reviewed. NB 28 list of unreviewed merges (some on the list did actually get a review but merged without anyone hitting approve - which is fine IMO).

[mbien]

compered this PR with previews version bumps and the changes look good to me

[neilcsmith-net]

@mbien this is not the place to be discussing how ASF code modification policies and guidelines are applied by us. I'm entirely with you on the need for practices that remove the need for reverts to happen. The initial PR was merged in a state of some contention, and after merge two people voiced the thought that it might need to be reverted. If we want to avoid reverts we also need to allow additional discussion to happen in such circumstances. My "post-merge approval mode" comment was a facetious way of saying that the merge status of the PR does not actually change the need to resolve certain questions and does not (and can not!) stop the conversation continuing.

[jtulach]

If you invoke the launcher with a relative path, that relative path is set to NETBEANS_USERDIR.

• Thanks for testing!
• created Use absolutize_paths to make userdir and path absolute #8998 follow up

[jtulach]

• I am using the latest builds with my changes daily
• It seems to me there are no problems related to
  • Use absolutize_paths to make userdir and path absolute #8998
  • Download launcher binaries as produced by #8756 #8959
  • Make sure netbeans in embedded terminal means itself #8756
• I can use the embedded terminal on Linux, Windows (Cygwin), Mac and talk to the surrounding netbeans from it
• Btw. I started to pass --userdir when launching NetBeans more, as now the shell talks to the right instances automatically