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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: whimboo, Assigned: aaronmt)

Tracking

Details

(Whiteboard: [addons-testday], )

Attachments

(3 attachments, 4 obsolete attachments)

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.
Summary: Exchange $1 .. $9 arglist with $@ → Exchange $1 .. $9 arglist with "$@"
Assignee

Comment 1

8 years ago
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
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.
Assignee

Comment 3

8 years ago
Posted patch Patch v1 - arglist (obsolete) — Splinter Review
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 "%*".
Assignee: nobody → aaron.train
Status: NEW → ASSIGNED
Attachment #509003 - Flags: review?(hskupin)
Summary: Exchange $1 .. $9 arglist with "$@" → Exchange $1 .. $9 arglist with "$@" and "%*"
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.
Attachment #509003 - Flags: review?(hskupin) → review-
Assignee

Comment 5

8 years ago
Attachment #509003 - Attachment is obsolete: true
Attachment #509096 - Flags: review?(hskupin)
Attachment #509096 - Flags: review?(hskupin) → review+
Assignee

Comment 6

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

First git commit in a while, if something's incorrect please ping me
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
This patch fixes the mentioned problems and removes the manual execution part.
Attachment #516892 - Flags: review?(aaron.train)
Whiteboard: [addons-testday]
Assignee

Comment 11

8 years ago
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.
Assignee

Comment 12

8 years ago
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
Attachment #516892 - Flags: review?(aaron.train) → review+
Attachment #509096 - Attachment description: Patch v2 - arglist → Patch v2 - arglist [checked-in]
Comment on attachment 516892 [details] [diff] [review]
Follow-up for Windows [checked-in]

Landed as:
https://github.com/whimboo/mozmill-crowd/commit/72a4d8849d2402a650bea378f964d58167269c2d
Attachment #516892 - Attachment description: Follow-up for Windows → Follow-up for Windows [checked-in]
Assignee

Comment 14

8 years ago
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 | ?
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?
Status: REOPENED → ASSIGNED
Assignee

Comment 16

8 years ago
Posted patch Follow-up for Linux/Mac (v1) (obsolete) — Splinter Review
Removes quotes from $@ and removes the dirname call for running activate
Attachment #516984 - Flags: feedback?(hskupin)
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.
Attachment #516984 - Flags: feedback?(hskupin) → feedback-
Assignee

Comment 18

8 years ago
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?
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"
Assignee

Comment 20

8 years ago
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
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.
Assignee

Comment 22

8 years ago
Posted patch Follow-up for Linux/Mac (v2) (obsolete) — Splinter Review
Ok, lets try this. Removed the manual case, and just output a help and execute command line arguments.
Attachment #516984 - Attachment is obsolete: true
Attachment #517322 - Flags: review?(hskupin)
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.
Attachment #517322 - Flags: review?(hskupin) → review-
Assignee

Comment 24

8 years ago
Posted patch Follow-up for Linux/Mac (v3) (obsolete) — Splinter Review
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.
Attachment #517322 - Attachment is obsolete: true
Attachment #517760 - Flags: review?(hskupin)
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.
Attachment #517760 - Flags: review?(hskupin) → review-
Assignee

Comment 26

8 years ago
Added $0 into message, output example:

Welcome to Mozmill. Use './test.sh mozmill --help' for assistance.
Attachment #517760 - Attachment is obsolete: true
Attachment #517872 - Flags: review?(hskupin)
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.
Attachment #517872 - Flags: review?(hskupin) → review+
Landed as:
https://github.com/whimboo/mozmill-crowd/commit/9bbca3ed60d06e0a5d17cbf4b82c49ce89ba9268

I'll update the environments and upload those to my people account now.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
No longer blocks: 623204
Blocks: 641897
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.