-
Notifications
You must be signed in to change notification settings - Fork 249
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
Pass toolchain and user env variables to make invocation #777
Pass toolchain and user env variables to make invocation #777
Conversation
@UebelAndre Would you be able to take a look at this PR? Without it, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! One thing I want to clarify just to make sure I understand the change before continuing 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for putting this together! The last thing before an approval though, would you be able to add some regression test? The rules as they are now are very prone to regressions so I think it'd be very helpful to have either a unit test in ./tests
or an integration test in ./examples
. Do you have a minimal repro case you'd be willing to share?
Could such an integration test also consist of a |
It depends on the size of the project. We have some fairly large 3rd party examples currently but we feel they're worth always building since they have complex build scripts to exercise the rules and are widely used by users of |
7b5682c
to
01190b5
Compare
I rebased this onto #796 and turned |
d6e1598
to
dc0e013
Compare
@jsharpe I reintroduced the wrapper and along the way fixed the test failure on Windows. |
This requires making the make_simple Makefile more realistic by * using CXX and forwarding it to the wrapper; * using CXXFLAGS instead of CXX_FLAGS and not overwriting its contents.
@UebelAndre Could you take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Sorry for the delay again! Thank you so much for putting this together and awesome job on identifying the windows issue 😄
Fixes #776.