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

Do not rely on launcher.sh being executable #184

Merged
merged 1 commit into from
May 20, 2017

Conversation

paulproteus
Copy link
Collaborator

@paulproteus
Copy link
Collaborator Author

This PR is untested so don't merge it quite yet.

@zarvox
Copy link
Collaborator

zarvox commented Aug 10, 2016

👍

Arguably, we should do the same for global-setup.sh, setup.sh and build.sh?

@paulproteus
Copy link
Collaborator Author

Agreed @zarvox .

I thought I did that a while ago; if not, then it's a PR or local patch of mine that's lost to the ages.

I want some vagrant-spk build bots!!!

@zenhack
Copy link
Collaborator

zenhack commented Apr 28, 2017

Ah, this is what you were talking about on #205; I was confused since I'd looked at the source and we weren't calling bash. This would negate the benefit of #206.

@zarvox
Copy link
Collaborator

zarvox commented Apr 29, 2017

Hmmm, I'd kinda like to revert #206 and merge this, since it seems to be a potential blocker for Windows users getting up and running, and there's an alternative approach that achieves the same end state (with one less file read, even!) that I mentioned on that ticket.

@zenhack
Copy link
Collaborator

zenhack commented Apr 29, 2017

I'm fine with that if it's what you want to do -- #206 was basically a hack to get around #205, the point of which was being able to automate the change you suggest.

Would be nice to be able to be a bit more flexible with what you can do with a stack in general. afaik there's no way to do "raw" stacks either, which you can also kinda pull off with Go (and will be more feasible as https://gihtub.com/zenhack/go.sandstorm gets more mature), and not having sandstorm-http-bridge in the spk shaves off 2-3 MiB.

Possible alternative: On windows, could just add a mount option that makes the executable bit be the default: https://stackoverflow.com/questions/35807568/vagrant-synced-folder-permissions/35821148#35821148

@zenhack
Copy link
Collaborator

zenhack commented Apr 29, 2017

If I'm understanding the bug correctly, we should hit the same problem trying to run a native executable when building on a windows box -- if stuff is non-exec, running /opt/app/app is going to fail. And there's no shell you can pass an ELF to. So perhaps this PR isn't general enough?

@zarvox zarvox merged commit c993ce6 into master May 20, 2017
@zarvox zarvox deleted the do-not-rely-on-shell-scripts-being-executable branch May 20, 2017 16:53
@zarvox
Copy link
Collaborator

zarvox commented May 20, 2017

Once you're editing the sandstorm-pkgdef.capnp, you're overriding what was initially specified on the spk init command line. I'm happy to say that if you want to eschew the shell, you can make sure your binary is marked +x by your build.sh and you get to modify sandstorm-pkgdef.capnp so that sandstorm launches your program directly.

@zenhack
Copy link
Collaborator

zenhack commented May 22, 2017

Right, but if /opt/app is a shared folder on a windows host, will doing chmod +x actually work? I thought the whole reason for this patch was that the executable bit wasn't doing what you want on windows hosts.

@zarvox
Copy link
Collaborator

zarvox commented May 23, 2017

Perhaps not. In that case, I guess the alternate approach is having build.sh place the binary in a folder outside of /opt/app, in the Linux-native filesystem (say, /usr/local/bin/ or something like that) and then having sandstorm-pkgdef.capnp reference that executable (which can safely have the +x bit)?

@zenhack
Copy link
Collaborator

zenhack commented May 23, 2017

That should work. However: do you have any objections to the mount option solution I proposed above? It seems like it would make a lot of things simpler.

(We also really ought to test this...)

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.

3 participants