update find_program_path to leave the env un-altered

[dmoody256]

The find_program_path should not alter the environment passed in. It should find the paths that the tool exists in and then let the caller decide if they want to append it, discard, store the paths, or etc.

The usage through the tools is to find the path from this function and then use a separate AppendENVPath call to append it.

This was also appending all paths to the PATH, when only one is needed.

Contributor Checklist:

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
• I have updated the appropriate documentation

[bdbaddog]

This is necessary if the path contains a space in it...

[dmoody256]

This is necessary if the path contains a space in it...

The java tools use this and returns paths that have spaces (C:\Program Files) and they work. Do you have an example of what you are referring too?

The tests passed on my appveyor build:
https://ci.appveyor.com/project/dmoody256/scons/builds/22097456

[bdbaddog]

Look at the git history for that section of code. Looks like it was for clang issue..

[bdbaddog]

Alternatively add a flag to specify behavior with reasonable default and only change for clang? But I think the change also fixed more than clang. All this was for appveyor environment.

[bdbaddog]

Which should be a reasonable approximation for some sane install paths I hope. (also appveyor can likely be a users CI environment at this point too)

[mwichmann]

Do we have a way of obtaining the appveyor paths, by the way? I was thinking of wix again... my wix installs all come from chocolatey, which takes care to make sure the things it installs appear in some known path, and my tests find it fine there. But scons is not finding it in the appveyor image unless I do the choco install....

[dmoody256]

Look at the git history for that section of code. Looks like it was for clang issue..
Alternatively add a flag to specify behavior with reasonable default and only change for clang? But I think the change also fixed more than clang. All this was for appveyor environment.
Which should be a reasonable approximation for some sane install paths I hope. (also appveyor can likely be a users CI environment at this point too)

Clang, as well as all the other tools tests pass with this change as well, apart from mingw, (which is getting worked in #3270). All the tools call AppendENVPath after find_program_path is called, so there is no reason to append it twice, and append all the additional default paths besides the first one that is finds a binary.

In any case, I can add a flag arg to specify the find_program_path to append to the path to make the function more useful.

Do we have a way of obtaining the appveyor paths, by the way? I was thinking of wix again... my wix installs all come from chocolatey, which takes care to make sure the things it installs appear in some known path, and my tests find it fine there. But scons is not finding it in the appveyor image unless I do the choco install....

I think the chocolaty bin directory is a good default for many tools, maybe even a tool wide default. I made a ticket asking appveyor support about appveyor WIX install location: https://help.appveyor.com/discussions/questions/34138-where-is-the-wix-binaries-located. I didn't find any info after a few google searches about the default install location for the WIX toolset.

[bdbaddog]

Here's what I've used. Basically a repo that I have dump out info about the filesystem.

https://github.com/bdbaddog/debug-appveyor-issues

[mwichmann]

I have an empty windows vm where I can just toss the changes later; I did an install of wix and it ended up in a place one probably would have guessed... C:\Program Files (x86)\WiX Toolset v3.11\bin. Now I can check why scons wouldn't find that. Silly windows, not only does every darn product gets its own installation path, but this one has the version string embedded in the path too.

[bdbaddog]

There's a wix PR #3204 you can look at with fixes to find it. Currently yield build with two different environments.

[bdbaddog]

The gotcha I've found (and I don't remember the specifics at this point).
If you have C:\program files\blah\bin\xyz.exe as you XYZ, the command line splitter is splitting on the space before fireing off the command one windows, but if you have the path in PATH, then it works fine. Of course the trick then is you have conflicting PATH...

There is not a simple workaround to force SCons to keep the XYZ token as a single token (in the case I was working on), and so the only reasonable solution (barring maintaining the integrity of the token) was to put it in the PATH and leave XYZ=xyz.exe.

[dmoody256]

The gotcha I've found (and I don't remember the specifics at this point).
If you have C:\program files\blah\bin\xyz.exe as you XYZ, the command line splitter is splitting on the space before fireing off the command one windows, but if you have the path in PATH, then it works fine. Of course the trick then is you have conflicting PATH...

There is not a simple workaround to force SCons to keep the XYZ token as a single token (in the case I was working on), and so the only reasonable solution (barring maintaining the integrity of the token) was to put it in the PATH and leave XYZ=xyz.exe.

That sounds like a different issue than find_program_path function adding a bunch of paths to the PATH. That's related to running commands with spaces in them. Is there an example I could reproduce with? The tools tests are passing with this change so maybe there is some case that is not tested.

[bdbaddog]

Yes. as I said no easy fix without big changes so leaving the path in PATH is the best current solution. So please don't change it..

[dmoody256]

Yes. as I said no easy fix without big changes so leaving the path in PATH is the best current solution. So please don't change it..

Ok, then I'll close this one

[mwichmann]

I'll check that on an Ubuntu VM. It looks like things are working, but I have enough doc building problems because, well, Fedora is not the same as Ubuntu it's hard to be sure.