630003 - Exchange $1 .. $9 arglist with "$@" and "%*"

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Crowd Extension, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

At the moment numbers for command line arguments are used. That limits us to only 8 additional parameters. I got a tip on IRC today to use $@ instead. That will give us unlimited arguments.

That's only for Linux and OS X, not sure about the Windows command shell. Would like to see this implemented for 0.2.

Comment 1

[Aaron Train [:aaronmt]]

http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/percent.mspx?mfr=true

For Windows, the script would have to use "shift" to move all the arguments in a batch file down by one. 

:foo
if "%0" == "" goto bar
        echo %0
        shift
        goto foo
:bar

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, as the document tells me there is also "%*" which should hold all parameters. Looks like it is similar to $@ on Linux and Mac. If that's the case we can go forward and fix it. Great to see that it works.

Comment 3

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - arglist

Changed the static hard coded argument count of $1 through $9 to use the "$@" variable on Unix based scripts which expands to all command-line parameters separated by spaces. On Windows, the equivalent is "%*".

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 509003 [details] [diff] [review]
Patch v1 - arglist

As the summary of this bug states we have to enclose the parameter array inside quotes to make sure that we preserve blanks.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - arglist [checked-in]

Comment 6

[Aaron Train [:aaronmt]]

Pushed to master
https://github.com/whimboo/mozmill-crowd/commit/235e0ef98a3dffe5b5a30322f429755bf4354f33

First git commit in a while, if something's incorrect please ping me

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> First git commit in a while, if something's incorrect please ping me

Only the quotes. But no reason to backout.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This patch is broken and doesn't work. My proposal with the quotes was wrong. Those have to be added when calling the command. We should run tests in any way and it seems those haven't been done.

We have to get this fixed. Aaron, can you make that change and also test on Linux? WOuld be great to get this in today because people complain about failures for the endurance testrun.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also %CMD% "%*" is wrong because calling the script with "run.cmd hg" transforms it to "hg hg". I would say we completely remove the manual execution and simply set all env variables with executing run.cmd without any options. So users can then run everything without the run.cmd script.

Same applies possible to Linux too.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Follow-up for Windows [checked-in]

This patch fixes the mentioned problems and removes the manual execution part.

Comment 11

[Aaron Train [:aaronmt]]

Ok I was just about to open take a look at this, but thanks for putting up a patch. Let me test it out in Windows.

Comment 12

[Aaron Train [:aaronmt]]

Comment on attachment 516892 [details] [diff] [review]
Follow-up for Windows [checked-in]

Looks good and tested on Windows. 

Tried it out with the following examples of multi argument passing:

C:\mozmill-env>run.cmd
"Welcome to Mozmill. Use 'mozmill --help' for assistance.

C:\mozmill-env>run.cmd hg --version
"Welcome to Mozmill. Use 'mozmill --help' for assistance.
Mercurial Distributed SCM (version 1.7.2)
(see http://mercurial.selenic.com for more information)

C:\mozmill-env>run.cmd hg clone http://hg.mozilla.org/qa/mozmill-automation
"Welcome to Mozmill. Use 'mozmill --help' for assistance.
destination directory: mozmill-automation
requesting all changes
adding changesets
adding manifests
adding file changes
added 33 changesets with 85 changes to 20 files
updating to branch default
19 files updated, 0 files merged, 0 files removed, 0 files unresolved

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 516892 [details] [diff] [review]
Follow-up for Windows [checked-in]

Landed as:
https://github.com/whimboo/mozmill-crowd/commit/72a4d8849d2402a650bea378f964d58167269c2d

Comment 14

[Aaron Train [:aaronmt]]

In the run.sh script for Linux and Mac, how do you want to continue to activate and run the virtual environment? Let the user run | source bin/activate | ?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would completely remove it and only allow the script to run via run.sh. The manual case always caused us problems. Windows users are lucky that we do not have to use a virtual environment. Does it mean you want to work on it now?

Comment 16

[Aaron Train [:aaronmt]]

Attachment: Follow-up for Linux/Mac (v1)

Removes quotes from $@ and removes the dirname call for running activate

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 516984 [details] [diff] [review]
Follow-up for Linux/Mac (v1)

>-PWD=$(dirname $0)
>-
>-# Change into the virtual environment
>-source $PWD/bin/activate

Why do you remove the virtualenv lines? That completely breaks the environment. 

> if [ $# -gt 0 ]; then
>     # start test-run
>-    "$@"
>+    $@
> else
>     PS1=$PS1"$ "
>     $(bash --norc)
> fi

This else case is not needed because that's the manual path. I would try to completely git rid of the if/else condition if possible.

Comment 18

[Aaron Train [:aaronmt]]

I suppose I misinterpreted comment #15 from my question in #comment 14. We want the script to change into the virtual environment automatically. With those lines re-inserted, and the else case condition removed, what is left to replace for Linux/Mac?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just check what happens with you run the script without params. We shouldn't fail at least. Also a help text like I have added on Windows would be nice. We can point to the help like "run.sh mozmill --help"

Comment 20

[Aaron Train [:aaronmt]]

If I remove the if-else and just execute arguments ($@), alongside adding a help message the following will happen.

1. Just running run.sh will output the help message and will not change our shell prompt, any set any path to the included binaries (mozmill/mercurial, etc). Exit status of 0.

2. Running run.sh alongside parameters will work fine, and output a help message. Such as, ./run.sh mozmill --v

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Exactly. That's wanted. See comment 15. In the manual case with a sub-shell you will never see failures unless you exit the sub-shell. That's pretty bad and makes it lesser usable.

Comment 22

[Aaron Train [:aaronmt]]

Attachment: Follow-up for Linux/Mac (v2)

Ok, lets try this. Removed the manual case, and just output a help and execute command line arguments.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 517322 [details] [diff] [review]
Follow-up for Linux/Mac (v2)

>+echo "Welcome to Mozmill. Use 'mozmill --help' for assistance."

Let us only show this help message when no options have been specified. Otherwise we collide with other help output from our automation scripts or Mozmill, which can let to confusion. Tested on OS X and it works fine.

Comment 24

[Aaron Train [:aaronmt]]

Attachment: Follow-up for Linux/Mac (v3)

v3, let's try it. Similar to before, when nothing but the script is specified at execution, the message is displayed. When additional arguments are specified, no message is displayed to conflict with other output.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 517760 [details] [diff] [review]
Follow-up for Linux/Mac (v3)

>+   echo "Welcome to Mozmill. Use 'mozmill --help' for assistance."

Mostly, but you forgot the run.sh or $0 part to include in-front of the mozmill command.

Comment 26

[Aaron Train [:aaronmt]]

Attachment: Follow-up for Linux/Mac (v4)

Added $0 into message, output example:

Welcome to Mozmill. Use './test.sh mozmill --help' for assistance.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 517872 [details] [diff] [review]
Follow-up for Linux/Mac (v4)

Looks mostly fine. But we can simplify the the echo line and can make sure to only output the filename and not the directory.

echo "Welcome to Mozmill. Use '$(basename $0) mozmill --help' for assistance."

As talked on IRC I will go ahead and check in the patch with this change included.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
https://github.com/whimboo/mozmill-crowd/commit/9bbca3ed60d06e0a5d17cbf4b82c49ce89ba9268

I'll update the environments and upload those to my people account now.