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

portability: /usr/bin/env bash #3574

Merged
merged 10 commits into from
Jul 29, 2020
Merged

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Apr 17, 2020

These changes close #3554

Basically sed -i 's|/bin/bash|/usr/bin/env bash|g' $(git grep --name-only '/bin/bash')

If tests pass I'll move onto docs and changelog.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Does not need tests (really hard).
  • Appropriate change log entry included.
  • (master branch) I have opened a documentation PR at portability: /usr/bin/env bash #3574.

@oliver-sanders oliver-sanders added this to the cylc-8.0.0 milestone Apr 17, 2020
@oliver-sanders oliver-sanders self-assigned this Apr 17, 2020
@oliver-sanders
Copy link
Member Author

Work in progress as Travis is throwing a wobbly over this one.

@oliver-sanders oliver-sanders changed the title /usr/bin/env bash portability: /usr/bin/env bash Apr 24, 2020
@oliver-sanders oliver-sanders marked this pull request as draft April 24, 2020 11:02
@oliver-sanders oliver-sanders force-pushed the usr-bin-env-bash branch 2 times, most recently from 3cbbe93 to 819c72b Compare April 30, 2020 13:24
@oliver-sanders
Copy link
Member Author

oliver-sanders commented May 1, 2020

I've got to the bottom of the test failures, the issue is that whilst this works fine:

$ /usr/bin/env -l

This does not:

#!/usr/bin/env bash -l

Works fine on my setup with MacOS and Bash5 but Ubuntu vanilla doesn't like it because in shebang lines arguments behave as though they were quoted apparently:

/usr/bin/env: ‘bash -l’: No such file or directory

Simple fix?:

-#!/usr/bin/env bash -l
+#!bash -l

I guess /usr/bin/env bash is effectively the same as bash anyway but it feels wrong not using an absolute executable path.

@hjoliver
Copy link
Member

hjoliver commented May 3, 2020

I guess /usr/bin/env bash is effectively the same as bash anyway but it feels wrong not using an absolute executable path.

I can't think of a reason why that statement is wrong, but if it is not wrong why does anyone ever need to use /usr/bin/env in that way?

Clearly #!bash will fail is bash is not in your PATH ... but so will #!/usr/bin/env bash because env searches for bash in your PATH ??

@oliver-sanders
Copy link
Member Author

oliver-sanders commented May 4, 2020

Sadly #!bash -l won't work.

After a docs dive:

  • Only BSD env permits multiple args/opts.
  • As of 8.30 (two years ago) GNU env does support multiple args/opts using the -S opt (like BSD).
  • According to Stack Overflow a "nice" universal solution is to re-invoke on the first line (I contest that this is "nice").

Options:

  1. Keep Cylc tied to system bash for job script invocations (which would be inconsistent and daft).
  2. Tie Cylc to GNU coreutils 8.30 (which would require additional install gunk).
  3. Re-invoke job scripts under login bash on line 2 (which is stupid).
  4. Determine the bash path pre-submission (e.g. in remote-init) then use this path when writing the job file (yuck).
  5. Manually source login files (might work but it wouldn't formally be a login session).

Options 1-5 seem pretty ugly, anyone got an option 6?

@oliver-sanders
Copy link
Member Author

Here's what option (3) would look like.

diff --git a/cylc/flow/etc/job.sh b/cylc/flow/etc/job.sh
index 28fd2d292..625e8da1f 100644
--- a/cylc/flow/etc/job.sh
+++ b/cylc/flow/etc/job.sh
@@ -1,5 +1,11 @@
 #!/bin/bash
 
+if [[ $1 == 'noreinvoke' ]]; then
+    shift
+else
+    exec "$(command -v bash)" "$0" noreinvoke "$@"
+fi
+

In retrospect it's probably not that bad since /usr/bin/env bash will involve a re-invocation anyway.

@oliver-sanders oliver-sanders force-pushed the usr-bin-env-bash branch 3 times, most recently from 8f7e4ae to 088e246 Compare May 11, 2020 14:06
@hjoliver
Copy link
Member

hjoliver commented May 11, 2020

Sadly #!bash -l won't work.

(Or, by experiment, not #!bash either)

The Wikipedia "Shebang" page says an absolute path is required:

Maybe that makes sense: when a script begins executing (before its content is passed the interpreter specified on the shebang line) it is being executed by the OS, not being interpreted by the shell that invoked it (after all, the shell that invoked it could be anything, potentially something with no executable search path mechanism).

@kinow
Copy link
Member

kinow commented May 11, 2020

Determine the bash path pre-submission (e.g. in remote-init) then use this path when writing the job file (yuck).

What if we leave the value we have now for linux, and replace it doing something like "if os == darwin"?

Could that work? Or maybe have a script for darwin?

@hjoliver
Copy link
Member

hjoliver commented May 11, 2020

Rrecapping, to make sure I understand. There are two problems:

  • no standard bash location
  • how to use a bash login shell for job scripts, given the above

The Wikipedia Shebang page pretty much says there's no perfectly portable solution for interpreter location, for a static script. So for bash location I guess our options are:

  • use #!/usr/bin/env bash
    • (but there's no universal standard env location either?!)
  • or install our own bash
    • (can only do via conda, so a pip-installed back-end will no longer be complete)
  • or figure out the system bash location on the fly, and write that to job scripts
    • (by elimination, this is the only foolproof solution?)

For login shell, we can't use #!/bin/bash -l because bash location isn't standard, but we can't use #!/usr/bin/env bash -l because some env versions won't take options. For this:

  • install our own option-taking env via GNU Core Utils 8.30
    • (as above - works with conda, but a pip-installed back end will no longer be complete)
  • source login scripts manually
  • have jobs script reinvoke themselves under login shells (described by @oliver-sanders above)
    • (seems nasty, but is foolproof?)
  • OPTION 6? Don't use a login shell for job scripts?
    • safer: the job script is more isolated from external settings
    • if users want, they can manually source their own login scripts etc. in job scripts

(I like the idea of not using a login shell, but too many users might be unknowingly relying on the existing behaviour?)

@hjoliver
Copy link
Member

I'd tentatively say that proposal is reasonable. But perhaps we could make it a (non-default) option to not reinvoke job scripts in login shells.

@hjoliver
Copy link
Member

The full solution ... possibly convert the whole thing to Python

I'm not entirely convinced about that, although it would be great to have it as an option. The problem is, task job scripts mostly do typical command-line-y stuff that is very easy to do in bash and essentially just reproduces what users do when developing and testing the executables that they want to run via Cylc: export variables, move files around, and run commands and executables. That's the one thing that (in small doses) bash is better (or at least easier) for than Python, and it seems a bit much to force everyone to figure out how to do it in Python. However, maybe I could be convinced 😬

@hjoliver
Copy link
Member

BTW this branch seems to have a few spurious changes?

$ git diff --stat master | grep -v '2 +-'
 .github/workflows/test.yml                         |   4 +-
 cylc/flow/etc/job.sh                               |   2 -
 cylc/flow/flags.py                                 |   1 +
 cylc/flow/job_file.py                              |  17 +-
 cylc/flow/suite_db_mgr.py                          |  25 +--
 tests/authentication/00-identity.t                 | 138 ++++++++++++++
 tests/authentication/01-description.t              | 149 +++++++++++++++
 tests/authentication/02-state-totals.t             | 190 ++++++++++++++++++++
 tests/authentication/03-full-read.t                | 193 ++++++++++++++++++++
 tests/authentication/04-shutdown.t                 | 190 ++++++++++++++++++++
 tests/authentication/05-full-control.t             | 199 +++++++++++++++++++++
 tests/authentication/06-suite-override.t           | 179 ++++++++++++++++++
 .../10-remote-suite-passphrase-cache.t             |  67 +++++++
 tests/cylc-run/02-format.t                         |   6 +-
 .../17-task-event-job-logs-retrieve-command.t      |   4 +-
 .../00-sigusr1/python/background_vacation.py       |   1 -
 tests/jobscript/00-torture/foo.ref-jobfile         |   7 +
 tests/restart/41-auto-restart-local-jobs.t         |   4 +-
 tests/rnd/00-test-battery-chunking.t               |  41 +++++
 tests/shutdown/14-no-dir-check.t                   |  14 +-
 655 files changed, 2039 insertions(+), 682 deletions(-)

@oliver-sanders
Copy link
Member Author

However, maybe I could be convinced

Discussion best placed somewhere else but here's a starter for ten:

  • Cylc comms client can live for the life of the job making messaging more efficient and reducing the overheads of job producing large number of messages (e.g. message on model tilmestep).
  • Permit user definable shell, changing the shell is extremely simple, scripts could be bash, ksh, python, ruby, whatever.
  • Byebye bash compatibility issues, we use Python (the version of which we can easily control).
  • Interface for breaking the one-to-many mapping between tasks-jobs.
  • Jobs could be light-weight Cylc schedulers (aka frames).
  • Advanced debugging.
  • Highly unit-testable / provable.
  • Environment caching for each stage in the job script (opens up possibility of partial re-runs).

@oliver-sanders
Copy link
Member Author

BTW this branch seems to have a few spurious changes?

Whoops, rebase mistake, now removed the accidentally re-added files along with the remaining content in tests/authentication - @datamel I think this directory should be empty now?

@hjoliver
Copy link
Member

hjoliver commented May 13, 2020

However, maybe I could be convinced

Discussion best placed somewhere else but here's a starter for ten:

OK, I'm convinced!

(Although I would still llump all of that into my "it would be great to have it as an option" comment - most users most of the time, I think, would still rather just use plain old bash to invoke a simple executable inside of their job scripts).

@oliver-sanders oliver-sanders marked this pull request as ready for review May 14, 2020 08:57
@oliver-sanders oliver-sanders modified the milestones: cylc-8.0.0, cylc-8.0a3 Jun 9, 2020
@oliver-sanders
Copy link
Member Author

Rebased.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Jul 14, 2020

@hjoliver we have approval from Dave, I'll leave you to whack the merge button when ready.

@oliver-sanders
Copy link
Member Author

Rebased.

@hjoliver
Copy link
Member

One of the new SoD tests failed here, I'll investigate...

@hjoliver hjoliver merged commit 5179d19 into cylc:master Jul 29, 2020
@hjoliver
Copy link
Member

One of the new SoD tests failed here, I'll investigate...

#3725

@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
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.

bash: allow user installation
5 participants