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

fix: duplicate ninja rules for v8 (cross-compiling) #185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moto-timo
Copy link
Contributor

avoids duplicate ninja rules for v8 used in host and target

From: minrk/node@5e533cb

Reported in nodejs/node#46690 and nodejs/node#37441

@cclauss
Copy link
Contributor

cclauss commented Feb 19, 2023

Please rebase.

@moto-timo moto-timo force-pushed the fix-v8-ninja-cross-compiling branch 2 times, most recently from 09e3ad8 to 58e88cf Compare February 20, 2023 17:20
@moto-timo
Copy link
Contributor Author

Rebased again.

avoids duplicate ninja rules for v8 used in host and target

From: minrk/node@5e533cb
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Reported in nodejs/node#46690
and nodejs/node#37441

Minor refactoring as recommended by Christian Clauss.

Signed-off-by: Tim Orling <tim.orling@konsulko.com>
@moto-timo
Copy link
Contributor Author

Rebased to pick up the xml.etree.cElementTree commit.

@moto-timo
Copy link
Contributor Author

@cclauss I believe I addressed your recommendations and rebase concerns. Anything else that needs to be done or any more discussion?

@cclauss
Copy link
Contributor

cclauss commented Feb 23, 2023

This pull request is rebased and the tests are all green. ✅ Thanks for that.

Now what we need is some maintainer who has enough expertise to review and approve or make suggestions.

@moto-timo
Copy link
Contributor Author

@cclauss great. Thank you for the response.

@ryzokuken
Copy link
Contributor

I can confirm that this goes with the default naming conventions of depot_tools and ninja, but I'd let someone else approve it for sure.

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.

None yet

4 participants