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

configure_in_place make a copy of the source code #596

Closed
wants to merge 5 commits into from

Conversation

jsharpe
Copy link
Member

@jsharpe jsharpe commented Mar 31, 2021

When using configure_in_place make a copy of the source code rather than symlinking as the configure script may modify the input source tree which shouldn't be modified.

@jsharpe jsharpe requested a review from UebelAndre March 31, 2021 20:47
@jsharpe jsharpe requested a review from oquenchil as a code owner March 31, 2021 20:47
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Is there an example (test) we could add for this? Seems like some behavior that could regress.

@jsharpe
Copy link
Member Author

jsharpe commented Mar 31, 2021

Is there an example (test) we could add for this? Seems like some behavior that could regress.

Yes I'll come up with an example that modifies the inputs in its configure script.

@jsharpe jsharpe force-pushed the configure_in_place branch from a6f420b to 62efa2f Compare March 31, 2021 22:57
# Note that perl -i would just replace the symlink
# if used directly

perl -i -pe 's/42/0/' $(readlink src/simple.c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

perl?! 😅 Does this work in RBE?

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears to! I'd default to sed but then I'd need to branch on os in the configure script to pass the correct arguments (or live with the extra src/simple.c-e file that would be created on macOS).
Unfortunately this example is still not breaking (it does with --spawn_strategy=standalone though)

@jsharpe jsharpe force-pushed the configure_in_place branch from 5bc240c to 31216dc Compare March 31, 2021 23:10
@jsharpe jsharpe marked this pull request as draft March 31, 2021 23:21
@github-actions
Copy link

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_foreign_cc!

@github-actions
Copy link

This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

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

Successfully merging this pull request may close these issues.

2 participants