Skip to content

exec in wrapper script if we aren't in the repl #306

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

Merged
merged 1 commit into from
Oct 14, 2017

Conversation

johnynek
Copy link
Contributor

@johnynek johnynek commented Oct 12, 2017

we had an internal system that didn't work when someone used the _wrapper script because we hadn't exec'ed.

This fixes it. Really, users should only use the wrapper.sh for the repl, but this is probably not clear, so this error is easy to make.

Alternatively, we could actually just not generate this file for non-repl targets, which I also think would work although it would be a break for people who have assumed it exists. This seems the most conservative change.

actually, the standard launcher calls this wrapper, so I think we do want this PR exactly as is.

@johnynek johnynek requested a review from ittaiz October 12, 2017 23:08
@ittaiz
Copy link
Contributor

ittaiz commented Oct 13, 2017 via email

@johnynek
Copy link
Contributor Author

Actually the java launching script requires the wrapper. I don’t want to refactor the java launching script we have now and I don’t see the gain immediately.

The problem we have is that we have tools that kill jobs, but without the exec we don’t kill the underlying process.

@johnynek
Copy link
Contributor Author

@ittaiz
Copy link
Contributor

ittaiz commented Oct 13, 2017

I agree about not going into a big refactor.
I think I have some knowledge gap about "exec".
I read in SO that:
"If the command exists and is executable, it replaces the current shell. That means that if exec appears in a script, the instructions following the exec call won't ever be executed (unless exec is itself in a subshell). exec never returns."
If exec never returns isn't that a problem?
This might be nonsense so forgive me if I'm talking about an unrelated issue.

Btw,
Can you extract this if into a private method with a meaningful name? I won't remember this in 3 months

@johnynek
Copy link
Contributor Author

the java script from bazel already calls exec. The problem is, it is calling exec on a script that does not call exec. So, we are breaking the intent and functionality of exec by our wrapper script.

The wrapper script needs to not exec on the repl, since the repl needs to fixup the stty settings. But in the case of other binaries, we want exec (which is how all the java processes work in bazel generally).

I don't know what you want exactly. Can you tell me where you want more methods? At the point of creating the wrapper script, I added the exec in a case it is sure to be safe (where there is nothing happening but a call to another program).

I will need your help to make it clearer, because it is not unclear to me.

(Also note, this is blocking us and causing us to resort to hacks of using sed on the inner scripts generated to manually insert the exec after the fact, which is obviously a bad state to be in. I'd like to get this fixed before we cargo cult this bad sed pattern all over our code and we try very hard to stay on master).

@ittaiz
Copy link
Contributor

ittaiz commented Oct 13, 2017

I meant another method in the bzl file to encapsulate the details of the if and the exec/empty-string
I think it's unclear but it might be due to this being tacked on the Bazel java wrapped.

If any of this makes sense and you have an idea that's great, otherwise merge it as is.

@johnynek
Copy link
Contributor Author

Since you say it is okay, I'm going to merge and let us file an issue. I don't really see that the code is cleaner by moving these few lines into a separate function as it is only called once. If you feel more strongly, you are welcome to make an edit that improves things which maybe I don't see right now.

@johnynek johnynek merged commit 6592f7e into master Oct 14, 2017
@johnynek johnynek deleted the oscar/exec-in-wrapper branch October 14, 2017 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants