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

build: split up cpplint to avoid long cmd lines #330

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Jul 6, 2017

  • 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
Contributor Author

kfarnung commented Jul 6, 2017

@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

refack commented Jul 6, 2017

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

@refack
Copy link
Contributor

refack commented Jul 6, 2017

Ohh and nodejs/node#13969

@kfarnung
Copy link
Contributor Author

kfarnung commented Jul 6, 2017

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
Contributor Author

kfarnung commented Jul 6, 2017

@refack
Copy link
Contributor

refack commented Jul 6, 2017

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
Contributor Author

kfarnung commented Jul 6, 2017

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.

@kfarnung kfarnung force-pushed the cpplint branch 2 times, most recently from f706d6a to 182fa10 Compare July 6, 2017 22:49
@refack
Copy link
Contributor

refack commented Jul 6, 2017

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

@refack refack 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
Contributor Author

kfarnung commented Jul 6, 2017

Thanks @refack!

Upstream PR: nodejs/node#14116

PR-URL: nodejs#330
Reviewed-By: Refael Ackermann <refack@gmail.com>
@kfarnung kfarnung merged commit 98b06ae into nodejs:master Jul 6, 2017
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jul 7, 2017
PR-URL: nodejs#330
Reviewed-By: Refael Ackermann <refack@gmail.com>
@kfarnung kfarnung deleted the cpplint branch July 7, 2017 00:41
@refack
Copy link
Contributor

refack commented Jul 9, 2017

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

@kfarnung
Copy link
Contributor Author

@refack Thanks!

Sign up for free to 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