-
Notifications
You must be signed in to change notification settings - Fork 900
Open file from embedded terminal, easily! #8756
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
base: master
Are you sure you want to change the base?
Conversation
npb.getEnvironment().put("DYLD_LIBRARY_PATH", "");// NOI18N | ||
|
||
envVars.put("LD_LIBRARY_PATH", "");// NOI18N | ||
envVars.put("DYLD_LIBRARY_PATH", "");// NOI18N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- it was always clear to me I need to set an environment variable to represent the value of
--userdir
- otherwise the
CLIHandler
won't be able to connect to the same NetBeans process - originally I thought I will have to make changes here, but ...
nb/ide.launcher/unix/netbeans
Outdated
@@ -38,6 +38,9 @@ cd "$progdir"/.. | |||
basedir=`pwd` | |||
cd "$old" | |||
|
|||
# make sure own launcher is on PATH | |||
PATH=$PATH:$progdir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- append directory where
netbeans
launcher is located to the PATH - for all processes launched from the IDE
- that way one can type
netbeans
in the terminal integration - and be sure it is defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why append and not prepend? If anyone has netbeans
in the PATH
, which will include anyone who has installed DEB or RPM versions, and is running an alternative version then this will launch the default instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also consider an environment variable for the current NetBeans process here? eg. typing $NB pom.xml
rather than netbeans pom.xml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why append and not prepend?
I wanted to play it safe, but you are right. Inside of NetBeans terminal, it is safer to have the same netbeans
executable invoked by default. Fixed in 914565a
typing $NB pom.xml
Typing netbeans
feels long, right? I also have a nb
symlink and I will update it to point to the right netbeans
executable or change it to alias nb=netbeans
.
I leave this "shortcutting" out of scope of this PR. The important change is the support for "inheriting user dir". Everything else is easy to script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing $NB pom.xml
Typing
netbeans
feels long, right? I also have anb
symlink and I will update it to point to the rightnetbeans
executable or change it toalias nb=netbeans
.
Actually, no, the length wasn't the reason. Putting the executable location in an environment variable rather than messing with the path at all keeps this more self contained and potentially safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might also be good to export the BASEDIR
value added in #8408 , although possibly under a NetBeans specific name? That would in most cases be enough to find the launcher, but separate environment variables for current basedir and current launcher locations would be good. That should cover what you need without altering the path for everything else too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If find the environment variable $NB
approach unnatural. I never used something like that before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it's unusual, but we need some way that a wrapper script can override this, and that is not path order dependent. Setting the PATH feels a little unsafe and unnecessary, and is going to break in interesting ways away from the zip bundle. The move to prepend rather than append fixes one problem but would introduce another, by bypassing the wrapper script for a system install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to do "unusual things" in this PR, thus:
- either I revert back to append - it works for me and causes no surprises
- or I don't modify PATH at all - hoping
netbeans
is already on PATH
The move to prepend rather than append fixes one problem
- this "problem"?
- it is not a problem as far as I can tell
- something gets invoked, something happens
- once the DEB or RPM launcher honors here in introduced
NETBEANS_USERDIR
environment variable... - then the behavior will even be exactly as expected
- thus I'd say appending is the best way to modify
PATH
(should this PR modifyPATH
at all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting an environment variable for the basedir and/or the executable file path isn't unusual. If using it directly feels unusual, then a separate script could be used to do this.
The problem is anything that already puts netbeans
on the PATH
. If you are running an alternative (zip) version with an appended PATH
then you'll get the default install. That may still do the right thing in most cases, once they have a version of NetBeans with the launcher that understands NETBEANS_USERDIR
included. I've no idea if other Linux packages play around with userdir
, although they may also not put netbeans
on the PATH
, but the DEB
and RPM
don't do anything.
If the netbeans
location is prepended, then using the installed RPM or DEB packages this would cause calls to netbeans
to go directly to /usr/lib/apache-netbeans/bin/netbeans
rather than via /usr/bin/netbeans
which would in some cases fail due to missing extra configuration.
In all, I have reservations about modifying the PATH
, but appending would be less likely to break things while not working in all scenarios. Environment variables feel a little more reliable.
@@ -138,6 +145,8 @@ launchNbexec() { | |||
then | |||
sh=/bin/bash | |||
fi | |||
NETBEANS_USERDIR=${userdir} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Store the value of
--userdir
to a system variable - so that child processes can pick it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- any objections against this change?
- if no, then TODO:
- document the new environment variable
- modify
netbeans.exe
to behave the same
- let's have a time for inception review now
- I plan to get this change into NetBeans 28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of the general principle - in fact I use this feature with the DEB package quite a bit, and alternative userdir support there would be great.
I have a few reservations about the implementation. Our packaging tool works by wrapping the launcher in various ways to meet different OS requirements. Some will already have netbeans
in the path. We need to ensure that this works for standalone and installed usage. And I think we need to ensure JAVA_HOME
is set in the parent to be picked up in the child process.
nb/ide.launcher/unix/netbeans
Outdated
@@ -38,6 +38,9 @@ cd "$progdir"/.. | |||
basedir=`pwd` | |||
cd "$old" | |||
|
|||
# make sure own launcher is on PATH | |||
PATH=$PATH:$progdir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why append and not prepend? If anyone has netbeans
in the PATH
, which will include anyone who has installed DEB or RPM versions, and is running an alternative version then this will launch the default instead?
|
No, the Windows launcher code is back in the main repository. It is built by the workflow at https://github.com/apache/netbeans/blob/master/.github/workflows/native-binary-build-launcher.yml When ready, the version numbers will need updating, binaries built from the workflow, and a release vote done - see #8408 and https://lists.apache.org/thread/491tvp6o44nj9wpmcpbdtcmt8rn0d9h8 EDIT : I'm currently -1 to the Have pinged @matthiasblaesing and @mbien for thoughts. Matthias also ran the last Windows launcher update and vote. |
I am somewhat mixed on this. On one hand I think that if a terminal behaves differently in the IDE it is essentially a bug (that is also why I think that the launcher should probably not export I am a bit worried about the can of worms this opens (why is javac, mvn, ant etc not in path too etc) and that it is set by default without opt-in or out. Maybe the solution is to store it in an env var and add a text field to the terminal settings where users can configure path extensions in the sense of I am wondering if
I agree, prepend would break existing user setups too, so if at all, it would have to be append IMO. (slightly related to this proposal would be: path selection -> right click -> open in NB or System for output and terminal as quality of life improvement) |
+1
I hadn't thought about setting an alias based on the environment variable, but that's a good idea. I think, set up something like But in the shorter term, the environment variable could still be invoked directly or used to set an alias manually. Having basedir in there too could allow for pointing aliases at some of those other things you mentioned, should people wish to. The launcher variable would need to be kept separate to the basedir, and only set if not already configured, so the various package scripts can use it to route through themselves.
Maybe. Some of the packages set |
netbeans pom.xml
in the embedded terminal would open the file in the IDE the terminal is embedded into--userdir
well