Skip to content

Cleanup wrapper script that launches IDE#1039

Open
alerque wants to merge 2 commits intopkulchenko:masterfrom
alerque:exec-not-background
Open

Cleanup wrapper script that launches IDE#1039
alerque wants to merge 2 commits intopkulchenko:masterfrom
alerque:exec-not-background

Conversation

@alerque
Copy link
Copy Markdown

@alerque alerque commented Feb 12, 2020

See commit comments.

Eventually a configure/build time flag should be added to set a system supplied Lua without having to patch at all, but this step in that direction at least makes things easier to start with.

@alerque
Copy link
Copy Markdown
Author

alerque commented Feb 12, 2020

Oops, branched from an old repository, I'll rebase on current master.

This is a shell anti-pattern and hinders integration into normal Linux
desktop usage. Only daemon launchers should background a job, and even
then only on request.

Instead we actually want to `exec` it, which has the effect of cleaning
up the wrapper script processes, making sure all the signals are
connected to the _actual_ foreground process, and passing exit codes and
such through when all is said and done.
For some distros (e.g. Arch Linux) packaging binaries compiled elsewhere
is a big no-no, and the more so when they are duplicates of something
the system has anyway. The current Arch Linux packaging for example has
to remove the generated binaries from the installation, then patch this
wrapper script to use the system Lua executable.

This change will make that process easier. First it will be possible to
run with a different Lua executable by setting an environment variable:

    LUA_EXECUTABLE=/usr/bin/lua zbstudio

Second, the coding style in the script will make it much easier to patch
to make the that choice stick.
@alerque alerque force-pushed the exec-not-background branch from 8c43e96 to cb0a998 Compare February 12, 2020 07:24
@alerque
Copy link
Copy Markdown
Author

alerque commented Feb 12, 2020

Rebased. Note the difference was that in 78229a9 you suppressed warnings. My rebase also reverts that. Suppressing everything on STDERR is also an anti-pattern. If launched from a zbstudio.desktop type launcher there will be no warnings anyway. If running it from the shell then STDERR needs to be passed through to the user (incidentally I am not seeing any warnings at all launching and editing a few files).

@pkulchenko
Copy link
Copy Markdown
Owner

Suppressing everything on STDERR is also an anti-pattern.

I agree, but it's done for practical purposes, as there are some messages from libraries that wxwidgets using, which clutter the output unnecessarily. Those of us who run debug builds and need to see assertions and other related messages, can easily tweak the launch script, but for everyone else I don't see a reason to get those messages.

I like the idea of providing lua executable path to launch with, but I think it needs to be specific to the IDE (in name), as there is a version dependency on (included) libraries.

@pkulchenko
Copy link
Copy Markdown
Owner

@alerque, how is LUA_EXECUTABLE set in your environment? Can we replace it with something like ZEROBRANE_STUDIO_EXECUTABLE? I'd prefer not to rely on LUA_EXECUTABLE, as it's too generic and may run into conflicts when the version of the executable and the one expected by the IDE don't match.

@pkulchenko
Copy link
Copy Markdown
Owner

@alerque, just to get back to my earlier question: do you need specifically LUA_EXECUTABLE name in the script or can we use an alternative (as suggested above)?

@alerque
Copy link
Copy Markdown
Author

alerque commented Apr 14, 2020

I believe this is because LUA_EXECUTABLE is what gets set by autoconf tools with the Lua detection macros we were using for SILE. I don't think there is anything magic about it.

In your case your CMakeLists.txt is already using finding Lua and setting the value in LUA_EXECUTABLE (so both GNU autoconf and CMake use the same variable) and you don't have to do anything further to get this substitution. You shouldn't use something specific to ZBS here, just go with the variable CMake is already setup to generate for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants