Skip to content

Conversation

@refack
Copy link
Contributor

@refackrefack 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-botnodejs-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
@refackrefack self-assigned this Jun 6, 2019
@refackrefack requested a review from jasnellJune 6, 2019 14:20
@targos
Copy link
Member

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
ContributorAuthor

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.

@refackrefackforce-pushed the delegate-lint-to-make branch 2 times, most recently from 3700683 to fe0a885CompareJune 6, 2019 16:21
@refackrefack requested a review from targosJune 6, 2019 16:21
@refackrefack added the review wanted PRs that need reviews. label Jun 6, 2019
@refack
Copy link
ContributorAuthor

@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...

@refackrefackforce-pushed the delegate-lint-to-make branch from fe0a885 to 5fe633eCompareJune 6, 2019 16:33
richardlau
richardlau previously requested changes Jun 6, 2019
Copy link
Member

@richardlaurichardlau 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
ContributorAuthor

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.

@refackrefackforce-pushed the delegate-lint-to-make branch from 5fe633e to bd1b911CompareJune 6, 2019 20:19
@refack
Copy link
ContributorAuthor

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

@richardlaurichardlau dismissed their stale reviewJune 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
ContributorAuthor

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?

@nodejsnodejs deleted a comment from nodejs-github-botJun 13, 2019
Copy link
Member

@joaocgreisjoaocgreis 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#28102Fixes: nodejs#28086 Reviewed-By: James M Snell <[email protected]> Reviewed-By: João Reis <[email protected]>
@refackrefackforce-pushed the delegate-lint-to-make branch from bd1b911 to 4b1bcaeCompareJune 13, 2019 13:03
@refackrefack removed the request for review from targosJune 13, 2019 13:04
@refackrefack removed the review wanted PRs that need reviews. label Jun 13, 2019
@refackrefack merged commit 4b1bcae into nodejs:masterJun 13, 2019
@refackrefack 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: #28102Fixes: #28086 Reviewed-By: James M Snell <[email protected]> Reviewed-By: João Reis <[email protected]>
@BridgeARBridgeAR mentioned this pull request Jun 17, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.windowsIssues 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

@refack@nodejs-github-bot@targos@richardlau@joyeecheung@joaocgreis@jasnell