Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Conversation

@sblom
Copy link

@sblom sblom commented Dec 21, 2012

@piscisaureus, would love your feedback this morning, since @izs would like to use this technology to help him with today's build.

Well-tested with VS 2010 Ultimate, but I still need to test this on a VS Express box, which will have to (ironically) be at the office tomorrow morning.

Meanwhile, I feel pretty good about this patch as a way to have git complain about modified files to those of us that have the WinSDK lying around (either from Visual Studio or otherwise), but to break the WinSDK dependency for folks that don't already have it installed.

Btw, I'm not happy about the obnoxious file names, but haven't found a way to change them. MSG00001.BIN, really?

@isaacs
Copy link

isaacs commented Dec 21, 2012

Landed on 841b7f5. Thanks, @sblom!

@piscisaureus I'd still love to get a review from you when you get back. If you object to this we can revert or refactor it, but it's good enough to get 0.9.4 out the door.

@isaacs isaacs closed this Dec 21, 2012
Copy link
Member

Choose a reason for hiding this comment

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

What if ctrpp.exe is not on the path? Shouldn't there be some kind of manual override?

@bnoordhuis
Copy link
Member

-1 from me. The files should be generated at configure or build time. Checking them in is deeply wrong.

@sblom
Copy link
Author

sblom commented Dec 22, 2012

@bnoordhuis, I'm with you in principle on the generated files thing. The reason we made this change is so that builders (@izs) with only the free Visual Studio Express can build Node without also going out and downloading the half-gig Windows SDK. So it's a bit of a crappy compromise, but all of us so far think we're doing the right thing.

@sblom
Copy link
Author

sblom commented Dec 22, 2012

If we end up deciding to keep this approach after discussing it, I'll make changes to address your other feedback, @bnoordhuis.

@piscisaureus
Copy link

I agree this is the best compromise.

@sblom I think configure should tell the user whether the "cached" stuff is being used or if it's generated fresh.

@bnoordhuis
Copy link
Member

$DEITY is going to kill kittens every time someone checks out the source but I guess it's the lesser of two evils. I rest my case safe for two things:

  1. The manual override.
  2. The location of the files. I'd much rather see them outside of src/ so it's perfectly clear that they're not "real" source files.

@piscisaureus
Copy link

$DEITY is going to kill kittens every time someone checks out the source but I guess it's the lesser of two evils. I rest my case safe for two things:

No I'm not!

@bnoordhuis
Copy link
Member

$DEITY is going to kill kittens every time someone checks out the source but I guess it's the lesser of two evils. I rest my case safe for two things:

No I'm not!

Sorry, didn't know I was dealing with the Lord of the Flies here. :-)

@sblom
Copy link
Author

sblom commented Dec 22, 2012

How about deps/gen (/winsdk ?) for location? Maybe tools/msvs/genfiles?

@TooTallNate
Copy link

+1 for tools/msvs/genfiles

@piscisaureus
Copy link

I concur with @TooTallNate

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.

5 participants