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.

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

build,win: delegate lint-cpp to make #28102

Merged
merged 1 commit into from
Jun 13, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 6, 2019

The script to collect and lint C/C++ files has become unruly, and carried a significant maintenance burned. IMHO delegating this task to make is the optimal solution image , since we already have MSYS as a prerequisite for developing.

Fixes: #28086

/CC @nodejs/build-files @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 6, 2019

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23838/
CI: https://ci.nodejs.org/job/node-test-pull-request/23857/

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jun 6, 2019
@refack refack self-assigned this Jun 6, 2019
@refack refack requested a review from jasnell June 6, 2019 14:20
@targos
Copy link
Member

targos commented Jun 6, 2019

We don't exactly have MSYS as a prerequisite. The building guide says "Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH."
On my machine, I installed Git with the additional Unix tools and it doesn't include make

@refack
Copy link
Contributor Author

refack commented Jun 6, 2019

I installed Git with the additional Unix tools and it doesn't include make

Oof... Yeah, I have extra MSYS tools 🤦‍♂
This was too good to be true.

@refack refack force-pushed the delegate-lint-to-make branch 2 times, most recently from 3700683 to fe0a885 Compare June 6, 2019 16:21
@refack refack requested a review from targos June 6, 2019 16:21
@refack refack added the review wanted PRs that need reviews. label Jun 6, 2019
@refack
Copy link
Contributor Author

refack commented Jun 6, 2019

@targos I vendored in a minimal MSYS make to help the Windows folks. It's just 19 files and 10MB in size. Let's discuss...

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

@targos I vendored in a minimal MSYS make to help the Windows folks. It's just 19 files and 10MB in size. Let's discuss...

But that's an additional 10MB that everyone is going to download (including non-Windows users). git really isn't a good place to store binaries.

Additionally we should not be adding GPL licenses/GPL-licensed files into the source tree (even if it applies to tooling and not the code we're actually building).

@refack
Copy link
Contributor Author

refack commented Jun 6, 2019

But that's an additional 10MB that everyone is going to download (including non-Windows users). git really isn't a good place to store binaries.

Yeah I've been thinking about this... I'm thinking of packing this as a separate archive, and adding a "win-lint-build" step.

Additionally we should not be adding GPL licenses/GPL-licensed files into the source tree (even if it applies to tooling and not the code we're actually building).

Ack. I was also reading reviews of GPL, in the context of this use case... AFAICT an independent pack should solve that.

@refack
Copy link
Contributor Author

refack commented Jun 6, 2019

@richardlau went in a different direction; best effort search in the Path and attempt calling wsl.

@richardlau richardlau dismissed their stale review June 6, 2019 22:08

binaries no longer included in the PR

@richardlau
Copy link
Member

@richardlau went in a different direction; best effort search in the Path and attempt calling wsl.

I've dismissed my objection as it's been addressed. I'm not explicitly approving because I'm -0.5 on a new requirement of GNU make to lint on Windows.

@refack
Copy link
Contributor Author

refack commented Jun 6, 2019

I'm -0.5 on a new requirement of GNU make to lint on Windows.

FTR, required just for linting C/C++, but I understand the hesitation.

I do believe that having a minimal make+bash easy to deploy package could allow us to eliminate much of the disparity and duplication between Makefile and vcbuild.bat, but this definatly needs some buy-in from Windows first devs.

@joyeecheung
Copy link
Member

Can we move the scripting to Python instead? I understand that on Windows we won't get the dependency update management from Make, but it has always been that way?

@nodejs nodejs deleted a comment from nodejs-github-bot Jun 13, 2019
Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

I agree with @joyeecheung, it would be better to have this in Python to be available everywhere without any added dependencies. But this is better than what is currently in master, we can work on the Python version later.

* look for GNU Make in the Path or use wsl

PR-URL: nodejs#28102
Fixes: nodejs#28086
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@refack refack removed the request for review from targos June 13, 2019 13:04
@refack refack removed the review wanted PRs that need reviews. label Jun 13, 2019
@refack refack merged commit 4b1bcae into nodejs:master Jun 13, 2019
@refack refack deleted the delegate-lint-to-make branch June 13, 2019 14:06
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
* look for GNU Make in the Path or use wsl

PR-URL: #28102
Fixes: #28086
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build/lint on Windows is slightly broken
7 participants