Fixed Urchin invocation problem in zsh and dash. Makefile "future-proofed" to take advantage of true cross-shell testing with Urchin, once available. #568
Conversation
|
Seems like one of the Travis jobs fell asleep :) The PR fixes #565 for me. |
| @@ -33,12 +33,14 @@ list: | |||
|
|
|||
| # Set of test-<shell> targets; each runs the specified test suites for a single shell. | |||
| # Note that preexisting NVM_* variables are unset to avoid interfering with tests, except when running the Travis tests (where NVM_DIR must be passed in and the env. is assumed to be pristine). | |||
| # CAVEAT: FOR NOW, ALL TESTS ARE INVARIABLY ALWAYS RUN WITH `sh`. THIS MUST BE FIXED ONCE `urchin` SUPPORTS THE `-s <shell>` OPTION - see https://github.com/creationix/nvm/issues/551: | |||
| # In the recipe command below, replace `$(URCHIN) -f test/$$suite` with `$(URCHIN) -s $$shell -f test/$$suite` | |||
There was a problem hiding this comment.
Instead of the caveat, could we just add the -s $SHELL now, and it would theoretically be a noop until the PR is merged?
There was a problem hiding this comment.
That would break the urchin invocation, because the current urchin doesn't yet recognize -s.
There was a problem hiding this comment.
urchin -f test/install_script -s dash works fine locally, it's just got naive option parsing, so the -f has to go first. Presumably with the PR, option ordering wouldn't matter.
There was a problem hiding this comment.
The only reason that works is a bug in the argument-validation code, which my urchin PR fixes.
However, my urchin PR requires that options come before non-option arguments, so your variant would break.
Do you feel that urchin should support options even after the (one and only) non-option argument?
There was a problem hiding this comment.
All command line tools should accept all dashed or double-dashed option pairs in any order, always.
There was a problem hiding this comment.
That's true for options, but not for positional arguments. I think you're mistaking -f to be the option for specifying the target directory, but it isn't: -f is a standalone option that means 'force running even if the target dir name doesn't contain the word "test"'.
Thus, test/install_script in the above is the (one and only) positional argument, so you'd effectively place the -s option after it, which is typically not supported (see http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html) - and I would recommend against it.
…s available, for true cross-shell testing. Conversely, a warning is issued if it is missing, because that implies that no actual cross-shell testing will take place. Also, the shell command that determines the set of available test suites is now POSIX-compliant.
|
The updated PR now does the following:
Thus, once the Urchin version that supports This strikes me as the most robust approach. |
mklement0
changed the title
Fixed Urchin invocation problem in zsh and dash. Makefile "future-proofed" to take advantage of true cross-shell testing with Urchin, once available.
The - currently - pointless invocations of Urchin itself from the various shells have been removed, pending the amendment to Urchin that will allow true cross-shell tests (see #551).
This fixes #565.