Latest version overwrites more environment than necessary

[abellgithub]

Version 1.7 only set environment variables that matched certain patterns. Version 1.8 sets all variables set by vcvarsall. This caused a failure on my application because the earlier code wouldn't set the PLATFORM variable, but the new code does (it would set it in a case-insensitive system, since "Platform" was one of the extracted variables).

I'm not sure that the new way is worse, but I'm leaving this here in case someone else is scratching their head trying to figure out what's up.

[pzhlkj6612]

Hi.

... This caused a failure on my application because the earlier code wouldn't set the PLATFORM variable, but the new code does (it would set it in a case-insensitive system, since "Platform" was one of the extracted variables).

I tested with both v1.7.0 and v1.8.0, and didn't found the exported variable PLATFORM , but Platform .

  env:
    ...
    Platform: x64
    ...

Maybe I don't fully understand what you said, could you please explain more? For example, how your application handles environment variables, what failure you got from your application, and so on.

[ilammy]

@abellgithub, uhh... Yeah, I have not foreseen such issue. I'm sorry for breaking your build. I believe you can work around this issue for now by using the 1.7 version, right?

@pzhlkj6612, on Windows environment variables are case-insensitive. So if the build already defines PLATFORM for its own unrelated purpose then msvc-dev-cmd would overwrite that value when it tries to set Platform because vcvarsall.bat wants that. And this will make the build upset.

---

Well, I guess what could be useful is to have a way to give users some control over what variables this action may or may not overwrite and set. For example, like this:

- uses: ilammy/msvc-dev-cmd@v1
  with:
    set-only-these-variables:
      - PATH
      - INCLUDE
      - LIBDIR

or like this:

- uses: ilammy/msvc-dev-cmd@v1
  with:
    never-touch-these-variables:
      - PLATFORM

(with variable names matched case-insensitively).

That way if one needs only some variables, they can ask only for them. If one already uses some variables and would not want MSVC to overwrite them, they could achieve that too.

[pzhlkj6612]

@ilammy Thank you for explanation. But sorry, I still can't get the point. v1.7.0 also exports Platform , right? Why @abellgithub 's build passed when using v1.7.0 ?

[abellgithub]

Sorry for the lousy explanation. Here are a couple of workflows that show the issue. The only difference is using version 1.7 vs. 1.8 of this action.

It's our fault, really, for using the PLATFORM env. var., but this took me a while to track down so I wanted to share for anyone else.

https://github.com/abellgithub/devenvtest/actions

[abellgithub]

I don't think you really need to add/change anything, but I guess if you have code that potentially changes behavior, bumping the major version would be good so as not to break things for people that, say, tie to @v1.

[ilammy]

Right, I have handled that poorly. I have been brooding over how to release those changes, thinking that maybe this isn't worth of becoming v2, as the syntax of the action inputs did not change, and adding any environment variable (include those added previously) could theoretically break someone's workflow. But yeah, this one should have been a v2.

Well okay, I got my lesson. If something can happen, it will happen. I'll be wiser next time.

[abellgithub]

Thanks!

Sometimes you get surprised by little changes that you don't think matter. At least I do.

[pzhlkj6612]

@abellgithub Thank you for posting the actions. I've noticed that the uppercased PLATFORM variable.

And, finally, I figured out why v1.7.0 works fine. I want to put my analysis here.

Look at the code:

const InterestingVariables = [
    // ...
    'Platform',
    // ...
]
//...
    for (let string of environment) {
        const [name, value] = string.split('=')
        for (let pattern of InterestingVariables) {
            if (name.match(pattern)) {
                core.exportVariable(name, value)
                break
            }
        }
    }

msvc-dev-cmd/index.js

Line 20 in d39d8f7

'Platform',

msvc-dev-cmd/index.js

Lines 140 to 145 in d39d8f7

	for (let pattern of InterestingVariables) {
	if (name.match(pattern)) {
	core.exportVariable(name, value)
	break
	}
	}

We know,

on Windows environment variables are case-insensitive. So if the build already defines PLATFORM for its own unrelated purpose then msvc-dev-cmd would overwrite that value when it tries to set Platform because vcvarsall.bat wants that.

Thus, due to the check of name.match(pattern) , v1.7.0 does not export a "renamed" Platform environment variable, so the original value is preserved.

I'm sorry that I didn't fully understand the code and asked some silly questions (e.g., this) before. 🤦‍♂️

[ilammy]

@pzhlkj6612, thanks for your analysis.

It slipped my mind that Platform was in the original list of exported variables. So it's just pure luck that PLATFORM did not get overwritten in v1.7 – if I had not forgotten about case-insensitivity of the variables and coded it ‘correctly’ then it would have still been overwritten. What a weird quirk.

I'm sorry that I didn't fully understand the code and asked some silly questions

There's nothing to be sorry for. That's how people learn. And you point out things that other people miss.

By the way, would you be interested in cooking a patch that lets the users to include/exclude certain variables? I think it wouldn't hurt to have this feature, just in case. And it could be a nice exercise to implement.