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

Switch lua scripts to use lua interpreter in PATH #789

Merged
merged 1 commit into from Sep 6, 2016

Conversation

Projects
None yet
5 participants
@SteVwonder
Copy link
Member

SteVwonder commented Sep 1, 2016

This is instead of using the hardcoded /usr/bin/lua interpreter.
Brings these lua scripts in line with other scripts that already do
so (see: src/cmd/flux-submit)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 1, 2016

While I could go either way on this (there are some drawbacks to using lua found in PATH), probably we should be careful not to touch the Lua scripts and tests that are pulled in from other projects like lua-affinity and lua-hostlist.

I imagine you are running on some platform without /usr/bin/lua, and thus the PR?

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Sep 1, 2016

Good point, I guess I should have opened an issue first rather than immediately submitting a PR (noted).

I actually am running on a system where /usr/bin/lua was Lua 5.2. I failed to get it working after a few hours, caved in and replaced the system Lua 5.2 with 5.1. So this isn't necessary for things to work on my system now, but it definitely was a problem.

If you want, I can remove lua-affinity and lua-hostlist from the PR. Otherwise, feel free to close.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 1, 2016

Coverage Status

Coverage decreased (-0.02%) to 75.06% when pulling f7c8159 on SteVwonder:fix-lua-interpreter into 1e403d9 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 1, 2016

Like I said, I could go either way (and sorry about lack of Lua 5.2 compatibility) I'm fine with use of #!/usr/bin/env lua I think in this case, though we've run into problems doing the same for python scripts (with system python scripts breaking in bad ways when somebody loads some other software that depends on a different python version) I don't think this will be a big issue for Lua though.

Ideally, the build system would embed the correct paths to supported Lua/python/sh into the files duriing build time, but this is always a pain because you have to work on a file script.lua.in then do make or configure to build script.lua to test it...

If you remove lua-affinity, lua-hostlist and any other imported scripts from the PR I think I'm fine to merge it (however, maybe get one other person's ACK first.)

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Sep 2, 2016

Any thoughts @garlick?

I don't mind either way since I found a workaround for my machine, but I think consistency would be good. Several lua scripts have /usr/bin/env lua at the top (i.e flux-cron, flux-submit, flux-wreck). So if using /usr/bin/lua is the decision, then maybe switching those 3 scripts to /usr/bin/lua should also be considered?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 2, 2016

It seems like #!/usr/bin/lua is better when flux-core is packaged (since dependencies can sort out which /usr/bin/lua is installed), and #!/usr/bin/env lua is better when flux-core is unpackaged, so a user without admin privs can get things working if /usr/bin/lua is wrong.

I guess I'd vote for #!/usr/bin/env lua for now, but I defer to @grondo's experience if he decides it should go the other way.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 2, 2016

Thanks for input @garlick!

I'm fine with #!/usr/bin/env lua for now, except for the scripts embedded from other projects as I mentioned above.

Thanks!

@SteVwonder SteVwonder force-pushed the SteVwonder:fix-lua-interpreter branch from f7c8159 to 499fe32 Sep 6, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 6, 2016

Current coverage is 74.69% (diff: 100%)

Merging #789 into master will decrease coverage by 0.02%

@@             master       #789   diff @@
==========================================
  Files           145        145          
  Lines         24960      24960          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          18648      18643     -5   
- Misses         6312       6317     +5   
  Partials          0          0          

Powered by Codecov. Last update 90fb199...499fe32

Switch lua scripts to use lua interpreter in path
This is instead of using the hardcoded `/usr/bin/lua` interpreter.
Brings these lua scripts in line with other scripts that already do
so (see: src/cmd/flux-submit)

@SteVwonder SteVwonder force-pushed the SteVwonder:fix-lua-interpreter branch from 499fe32 to 7c67adc Sep 6, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 6, 2016

Coverage Status

Coverage decreased (-0.02%) to 75.009% when pulling 7c67adc on SteVwonder:fix-lua-interpreter into 90fb199 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 6, 2016

Coverage Status

Coverage decreased (-0.05%) to 74.978% when pulling 7c67adc on SteVwonder:fix-lua-interpreter into 90fb199 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 6, 2016

Thanks, @SteVwonder !

@grondo grondo merged commit d1da47b into flux-framework:master Sep 6, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 74.978%
Details

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

@SteVwonder SteVwonder deleted the SteVwonder:fix-lua-interpreter branch Feb 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.