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.

Sign up for GitHub

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

Jump to bottom

win: try to find VS2017 tooling #30

Closed
wants to merge 1 commit into from
Closed

win: try to find VS2017 tooling #30

wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Jan 10, 2017

Using the MS recommended way of looking in the registry
Work with all flavors of VS2017, even the minimal "C++ Build Tools"
(obviously npm run test passes)

@refack
Copy link
Contributor Author

refack commented Feb 2, 2017

@indutny review?

@refack refack force-pushed the detect-vs2017 branch 2 times, most recently from 142b10b to f218e76 Compare February 2, 2017 14:16
@refack
Copy link
Contributor Author

refack commented Feb 2, 2017

@indutny BTW don't give up on this project!
I hate the gyp dependency. It's virtually an abandoned project, it drags in python, and it just makes the build environment too brittle (more so for "native modules").

indutny
indutny requested changes Feb 2, 2017
View reviewed changes
Copy link
Owner

@indutny indutny left a comment

Choose a reason for hiding this comment

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

What is a Setup.Configuration.exe file about? Where did it come from, is it possible to just use command-line tools provided with the system?

lib/gyp/platform/win.js Outdated
return envFile;

Copy link
Owner

Choose a reason for hiding this comment

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

Superfluous change

test/platform-win-test.js Outdated
@@ -13,38 +13,38 @@ describe('gyp.platform.win', () => {

it('should remove prefix `-l`', () => {
assert.deepEqual(
win.adjustLibraries([ '-llib1.lib', 'lib2.lib' ]),
[ 'lib1.lib', 'lib2.lib' ]);
win.adjustLibraries(['-llib1.lib', 'lib2.lib']),
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason to change this?

@refack refack force-pushed the detect-vs2017 branch 2 times, most recently from 124dbc1 to 51d2ad9 Compare February 3, 2017 01:43
@refack
Copy link
Contributor Author

refack commented Feb 3, 2017

So the "Setup.Configuration.exe" tools comes from Microsoft's recomanded way to find visual studio components actualy this repos, self compiled. It's a tiny tools that just calls a COM server, and obviously only works when VS2017 is installed but fails gracefully.
I did a similar PR at nodejs/node-gyp/#1103

P.S. White space changes were reverted.

@indutny
Copy link
Owner

indutny commented Feb 3, 2017

@refack I have very mixed feelings about distributing .exe file. Is there any way to port the functionality to plain JS?

@refack
Copy link
Contributor Author

refack commented Feb 3, 2017

I totally get what you're saying, I'll dig in more...

@indutny
Copy link
Owner

indutny commented Feb 3, 2017 via email

@indutny
Copy link
Owner

indutny commented Feb 3, 2017

Given that this indeed speaks to COM, I'm not sure if there is any easier way to do it without having either .exe file or custom binding... That .exe file is distributed under MIT license as far as I can tell, perhaps we should go with it.

What do you think?

@refack
Copy link
Contributor Author

refack commented Feb 3, 2017 •
edited
Loading

There is a way to talk to COM via powershell and even with JScript via wscript (Used to practice the dark art of JScript in the early 2000's). So it will still be an exec but with builtin tools.

Give me the rest of the day...

@indutny
Copy link
Owner

indutny commented Feb 3, 2017

Sure! Thanks!

@refack
win: try to find VS2017 tooling
0dea1ae 
Using the MS recommended way (COM call)*
Work with all flavors of VS2017, even the minimal "C++ Build Tools"

*https://blogs.msdn.microsoft.com/heaths/2016/09/15/changes-to-visual-studio-15-setup/
@refack
Copy link
Contributor Author

refack commented Feb 3, 2017

Cracked it! 🥇

@refack refack mentioned this pull request Feb 6, 2017
Closed
@refack
Copy link
Contributor Author

refack commented Feb 9, 2017

made obsolete by #33

@refack refack closed this Feb 9, 2017
@refack refack deleted the detect-vs2017 branch February 15, 2017 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indutny indutny indutny requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@refack @indutny