Skip to content
This repository was archived by the owner on Oct 15, 2020. It is now read-only.

Conversation

@kfarnung
Copy link
Contributor

  • Added a separate cpplint run for chakrashim
  • Added setlocal to the start of the script to prevent environment
    variables from leaking out
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@kfarnung
Copy link
ContributorAuthor

@refack Do you know of any reasons why we can't take the setlocal upstream? That should make the script cleaner since the environment variables get reset when the script exits.

@refack
Copy link
Contributor

vcvarsall.bat 😞
But I already promised nodejs/node#14115 (comment) to work on nodejs/node#12310

@refack
Copy link
Contributor

Ohh and nodejs/node#13969

@kfarnung
Copy link
ContributorAuthor

Hmm, I'm not seeing any issue with the addition. Everything spawned by the batch script inherits the full set of environment variables, it's just that once the script ends the variables are restored to their prior state.

Are there any cases where we run vcbuild.bat and depend on the leaked variables on subsequent runs?

/cc @joaocgreis

@kfarnung
Copy link
ContributorAuthor

@refack
Copy link
Contributor

So until about a week ago DISTTYPEDIR was explicitly leaked.
And having vcvarsall.bat setup the dev environment is very handy.
So the CI will probably pass, but it's still semver-major (since vcbuild.bat if used downstream by a bunch of vendors)

@kfarnung
Copy link
ContributorAuthor

Ok, makes sense, I'll just remove that for now. I was seeing some weird behavior where it seemed like some variables were being leaked and appended to until they were too long to be used. I'll just keep an eye out for a repro and try to debug.

@kfarnungkfarnungforce-pushed the cpplint branch 2 times, most recently from f706d6a to 182fa10CompareJuly 6, 2017 22:49
@refack
Copy link
Contributor

I was seeing some weird behavior where it seemed like some variables were being leaked and appended to until they were too long to be used. I'll just keep an eye out for a repro and try to debug.

It's a known bug of VS2015's vcvarsall.bat, I accidently removed a guard around it in nodejs/node@780acc2

Copy link
Contributor

@refackrefack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you upstream this

@kfarnung
Copy link
ContributorAuthor

Thanks @refack!

Upstream PR: nodejs/node#14116

@kfarnungkfarnung merged commit 98b06ae into nodejs:masterJul 6, 2017
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jul 7, 2017
@kfarnungkfarnung deleted the cpplint branch July 7, 2017 00:41
@refack
Copy link
Contributor

@kfarnung the "line too long" issue fix landed nodejs/node#13765

@kfarnung
Copy link
ContributorAuthor

@refack Thanks!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kfarnung@refack