Fix for issue 1876 - Updated the batch file to replace "%" in arguments with an arbitrary …

[Duncan-Howe]

…placeholder "##PC##" during the "collectargs" routine and then restoring the "%" signs prior to passing to the scancode executable

Signed-off-by: Duncan Howe [email protected]

Fixes #1876

Tasks

• Reviewed contribution guidelines
• PR is descriptively titled 📑 and links the original issue above 🔗
• Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
  Run tests locally to check for errors.
• Commits are in uniquely-named feature branch and has no merge conflicts 📁

[sschuberth]

Hmm, maybe before trying to fix the logic to "collect all command line arguments in a variable" we should ask @pombredanne why this collecting is even necessary. More concretely, why don't we just use %* instead of creating SCANCODE_CMD_LINE_ARGS?

[pombredanne]

@Duncan-Howe Thank you ++
I wonder how to add a test for this?
@sschuberth I am no expert in batch files and cmd: this was to "slurp" arguments is as old as scancode itself.
If I recall correctly this is an idiom that I first saw in Tomcat https://github.com/apache/tomcat80/blob/trunk/bin/startup.bat#L47
I am fine with replacing this with anything that works nicely everywhere :)

[pombredanne]

For reference #1876

[pombredanne]

@Duncan-Howe Do you mind to also update the changelog?

[Duncan-Howe]

@pombredanne What version number should I use as the heading for CHANGELOG or would vNext be OK?

[sschuberth]

I am no expert in batch files and cmd: this was to "slurp" arguments is as old as scancode itself.

If the aim is to forward all args passed to the batch file, then IMO %* could be used and no complicated replacement of placeholders should be necessary. The story becomes more complicated if you want to forward e.g. all but the first argument, but this is not the case here AFAIU the code. BTW, the Tomcat code says to "Get remaining unshifted command line arguments", but there actually are no unshifted arguments before that line, so also the Tomcat code could be much simpler IMO. Tomcat probably also copied this from somewhere without exactly knowning what they are doing 😉

[sschuberth]

So, @Duncan-Howe, would you mind giving the %* approach a try?

[Duncan-Howe]

@sschuberth Sure thing - I will refactor the batch file to use "%*" to pass the arguments through

[pombredanne]

@pombredanne What version number should I use as the heading for CHANGELOG or would vNext be OK?

@Duncan-Howe Yes, that would work fine 👍

[pombredanne]

Tomcat probably also copied this from somewhere without exactly knowning what they are doing wink

I would call this me doing cargo culting at its best :D
This is 15 years old :P https://github.com/apache/tomcat/blob/94400a237d15768ca227dd8c0f4dd5716b48e388/bin/startup.bat#L33

[Duncan-Howe]

Updated scancode.bat to remove a number of lines of code that weren't really doing much :-)
Now the arguments are passed straight through using the "%*" syntax so any number of arguments should work (validation of the arguments is delegated to the ScanCode executable)

[Duncan-Howe]

The updated batch file seems to perform in exactly the same way as previously when executed from ORT and produces the same result files for my own application scans

[sschuberth]

The updated batch file seems to perform in exactly the same way as previously

Great! Thanks a lot for getting rid of this cruft in the batch file 😄

[pombredanne]

LGTM and merging now!
Thank you ++