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

Add support for NetBSD to node-gyp #803

Closed
wants to merge 4 commits into from
Closed

Add support for NetBSD to node-gyp #803

wants to merge 4 commits into from

Conversation

TomFreudenberg
Copy link

Currently node-gyp does not allow to use packages like npm install netroute in case of miss-identification OS => linux on NetBSD.

I updated and inserted the necessary checks.

@bnoordhuis
Copy link
Member

I appreciate the effort but can you send the changes to gyp/ to the upstream gyp project first? I'll be happy to cherry-pick the patch once it lands there. Let me know if you want me to walk you through the process of submitting a patch.

@TomFreudenberg
Copy link
Author

Hi sure, may you give me the link to that git repo, so I will fork from there and push my changes again

@bnoordhuis
Copy link
Member

It's https://chromium.googlesource.com/external/gyp. You'll need git-cl from depot_tools to submit patches. Can you paste the link to the CL (changelist - git-cl will print it after it uploads the patch) in the issue?

@TomFreudenberg
Copy link
Author

I checked the git out from chromium but file "addon.gpyi" is not available (anymore)?

See https://github.com/4commerce-technologies-AG/node-gyp/commit/553ecb95ffd29f3beb1167a596868706e741df94

Is this something what only is inserted in this repo?

@bnoordhuis
Copy link
Member

Correct. The gyp/ directory in this repo corresponds with the upstream gyp repo.

@TomFreudenberg
Copy link
Author

Ok, so I send the 3 commits to them and if merged, you pick the missing one here ?

@bnoordhuis
Copy link
Member

Yep. Can you post the link to the CL when you're done?

@TomFreudenberg
Copy link
Author

Hi, do I have to run that review process ?

git cl upload --send-mail

@TomFreudenberg
Copy link
Author

Or may I send it to the commit queue?

git cl set_commit

@bnoordhuis
Copy link
Member

I normally do git cl upload HEAD^, then open the CL in a browser and pick reviewers manually. I think thakis@ does most of the reviews.

@TomFreudenberg
Copy link
Author

How do I pick reviewers?

@TomFreudenberg
Copy link
Author

The link is at:

https://codereview.chromium.org/1421073004

@TomFreudenberg
Copy link
Author

Hopefully I made the job right and published that to the reviewers ....

What a complicated process ... I asked myself more than once during that procedure to stop and do not publish it back. ;-)

@bnoordhuis
Copy link
Member

I agree. It's probably an effective first-pass filter for weeding out low quality contributions, though.

(I'm half tongue in cheek. It's just the workflow that google uses for their projects.)

@TomFreudenberg
Copy link
Author

Do you now what time to expect until someone will take the review?

@bnoordhuis
Copy link
Member

Probably in the next few working days.

@TomFreudenberg
Copy link
Author

Hi Ben @bnoordhuis

Do I need to check if my request on chromium is correctly announced? I am wondering that I did not get any kind of feedback. Something is normal or do I need to do something to make it work?

@bnoordhuis
Copy link
Member

Did you pass --send-email to git cl upload or click Publish+Mail in the web interface? I don't see the email on the gyp-developers@ mailing list.

@TomFreudenberg
Copy link
Author

Yes I used publish - see:

https://codereview.chromium.org/1421073004/#msg3

That was my publish post.

@bnoordhuis
Copy link
Member

Left a comment on the CL and now it shows up on gyp-developers@.

@TomFreudenberg
Copy link
Author

Thanks Ben!

@TomFreudenberg
Copy link
Author

Hi Ben, I have to ask again for the processing on chromium. As by now 2 person had LGTM signed do I have to act in any way or just stay and wait ;-)

@bnoordhuis
Copy link
Member

I'd give it a few more days. Going by the traffic on the mailing list, things have been a little quiet this week.

@TomFreudenberg
Copy link
Author

Ok, thanks for feedback ... so I will wait some more days.

@TomFreudenberg
Copy link
Author

It is a pity ... it seems not to move on on chromium side ...

@TomFreudenberg
Copy link
Author

Hi Ben ( @bnoordhuis )

Code is now landed on chromium. Do you pick https://github.com/4commerce-technologies-AG/node-gyp/commit/553ecb95ffd29f3beb1167a596868706e741df94 by yourself or do I have to do something in addition?

Cheers
Tom

@bnoordhuis
Copy link
Member

I've opened #831 for upgrading gyp wholesale.

@bnoordhuis
Copy link
Member

Fixed in v3.2.1, thanks Tom.

@TomFreudenberg
Copy link
Author

Hey Ben,

thanks for help on this tasks. Great that these issues are out now.

Tom

P.S.: You will close the issues as you decide, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants