Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PATH order is not correct #10

Closed
nickhutchinson opened this issue Dec 28, 2020 · 6 comments · Fixed by #12
Closed

PATH order is not correct #10

nickhutchinson opened this issue Dec 28, 2020 · 6 comments · Fixed by #12

Comments

@nickhutchinson
Copy link

As of v3, this GitHub action does not add entries to PATH in the right order, because it iterates over the PATH entries set by VsDevCmd and calls core.addPath() for each one. This reverses their order -- core.addPath prepends PATH entries!)

One result of this is that if you request the x64_x86 toolchain (host_arch=x64 arch=x86), you get the following entries in PATH:

\path\to\VC\Tools\MSVC\14.28.29333\bin\HostX64\x64;\path\to\VC\Tools\MSVC\14.28.29333\bin\HostX64\x86

These entries are back to front, and this causes the wrong cl.exe being found.

Also, it's important to note that VsDevCmd also both appends and prepends entries to PATH. cl.exe and friends go the front, but other tools like the Visual Studio-bundled CMake and Ninja are appended at the end (so existing installs take precedence).

I suspect this action shouldn't be special-casing PATH, and should instead respect the value set by the vsdevcmd script.
@seanmiddleditch
Copy link
Owner

This reverses their order -- core.addPath prepends PATH entries!)

It prepends them after the action. Each path given to addPath is appended to the paths file (https://github.com/actions/toolkit/blob/c861dd8859fe5294289fcada363ce9bc71e9d260/packages/core/src/file-command.ts#L21) which is only applied at the end of the action's execution.

Though perhaps GitHub adds the entries from that file in reverse order?

I suspect this action shouldn't be special-casing PATH, and should instead respect the value set by the vsdevcmd script.

I'm not sure I follow the suggestion here.

The action has to treat path specially since GitHub uses a different API to set path values vs other kinds of environment variables.

@nickhutchinson
Copy link
Author

It prepends them after the action. Each path given to addPath is appended to the paths file (https://github.com/actions/toolkit/blob/c861dd8859fe5294289fcada363ce9bc71e9d260/packages/core/src/file-command.ts#L21) which is only applied at the end of the action's execution.

Though perhaps GitHub adds the entries from that file in reverse order?

Yes, that seems to be what's happening. The docs suggest:

The runner will prepend the path given to the jobs PATH.

So if vsdevcmd generates a Path of A;B;<OldPath>;C, this GitHub action will call core.addPath("A"); core.addPath("B"); core.addPath("C"); and the resulting Path will be C;B;A;<OldPath>.

The action has to treat path specially since GitHub uses a different API to set path values vs other kinds of environment variables.

I don't think anything actually stops you calling exportVariable() for "Path" just like any other environment variable. addPath() function does more harm than good here I think.

@seanmiddleditch
Copy link
Owner

Would be open to a PR to fix this. I'll get to it eventually, but it's low-priority for me so far.

(Frankly, I wish Microsoft would just release their own microsoft/setup-vsdev Action so I can deprecate this one...)

compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
6f8b4d5 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
720291a 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
d78e10e 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
e166e6a 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
73ceae8 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
a5631a0 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
e552f57 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
7f33946 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
427c854 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
b4b3a68 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
e57507f 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
c756839 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
@compnerd
Copy link
Contributor

Seems that I am doing something incorrectly as no matter what change I do it doesn't seem to take effect (e.g. added logging has no impact).

compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
a65cd1b 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
696eb0c 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
c674eb5 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
1e4ca71 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
46dad43 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
1dbf244 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
87a1a11 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
e498687 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
0d77c04 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
658d813 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
ae25a2e 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
30736f2 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
compnerd added a commit to compnerd/gha-setup-vsdevenv that referenced this issue Oct 23, 2021
@compnerd
Export the Path variable rather than appending to it
4add743 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: seanmiddleditch#10.
@compnerd compnerd mentioned this issue Oct 23, 2021
Merged
@seanmiddleditch
Copy link
Owner

For reference, there's an upstream bug and PR about the behavior, actions/toolkit#270, for a long-term fix (... if it ever gets merged/accepted)

@compnerd
Copy link
Contributor

compnerd commented Oct 24, 2021 •
edited
Loading

Well, at least the right compiler gets invoked with #12. I'll see if I get the motivation to update the docs for developers like me who aren't particularly comfortable with npm. Realized later that I needed to use npm script-run build as well.

seanmiddleditch pushed a commit that referenced this issue Oct 24, 2021
@compnerd
Export the Path variable rather than appending to it (#12)
7665a85 
The `addPath` will invert the path ordering as each entry is appended to the
environment file, which is then reversed and prepended to the environment at the
end of the action.  This results in the additions being in reverse order.
However, since we know that `vsdevcmd` will adjust the path on both ends, simply
prefer to use the `exportVariable` rather than try to be clever to adjust the
path via `addPath`.

Fixes: #10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Export the Path variable rather than appending to it compnerd/gha-setup-vsdevenv
3 participants
@compnerd @nickhutchinson @seanmiddleditch