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

Ensure we are cross compiling when any cross env variables are set. #1514

Merged
merged 1 commit into from
Mar 28, 2021

Conversation

jameshilliard
Copy link
Contributor

Potential partial fix for #1513.

@kngwyu
Copy link
Member

kngwyu commented Mar 23, 2021

Thanks,

  • Did you confirm that this works for your case?
  • What do you mean by 'partial'? What problem do you think remains?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

@jameshilliard thanks for this - can you check if this patch in conjunction with the suggested LD_LIBRARY_PATH setting might provide enough of a working solution to unblock you? (#1513 (comment))

This could also use a CHANGELOG entry - e.g. "Fixed - Always use cross-compiling configuration if any of the environment variables are set".

@jameshilliard
Copy link
Contributor Author

Did you confirm that this works for your case?

At the moment I haven't been able to fully validate this properly due to the cargo issue, the rust toolchain is rather exotic and buildroot's rust support is different from most other language toolchains we use(mostly due to having to use prebuilt rust toolchains since rust can't easily be bootstrapped due to the rust requires rust to build issue) so I'm having difficulty testing stuff that requires modifications to stuff deep in the dependency tree(I'm quite familiar with python package cross compilation in buildroot since I help maintain a good number of python packages there but have not worked much with rust). In any case having this merged should make it easier for me to isolate the cargo issue since env variables are generally quite easy to control with buildroot.

What do you mean by 'partial'? What problem do you think remains?

Well it looks like the main other issue is the cargo issue, but this best I can tell fixes the most obvious cross compilation detection logic issue I saw in pyo3. This at least shouldn't cause regressions since these env variables seem to be exclusively used in cross compilation enabled codepaths, so if any of them are set we should be able to safely assume that cross compilation should be unconditionally enabled.

can you check if this patch in conjunction with the suggested LD_LIBRARY_PATH setting might provide enough of a working solution to unblock you?

Well the LD_LIBRARY_PATH workaround doesn't really look like it's viable to upstream for buildroot to me as we do in fact support compiling for x86_64 targets with newer instructions sets than the build host(buildroot uses a custom autobuilder CI system with older build hosts to ensure this sort of thing isn't broken). I'll play around with this a little though.

This could also use a CHANGELOG entry - e.g. "Fixed - Always use cross-compiling configuration if any of the environment variables are set".

Added.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 I'm satisfied that this patch is correct; and while there's other upstream issues at least we can get one step closer with this in place.

Thanks for the contribution!

@jameshilliard
Copy link
Contributor Author

👍 I'm satisfied that this patch is correct; and while there's other upstream issues at least we can get one step closer with this in place.

Great, by the way I made a few attempts to go through the cargo codebase to try and isolate how it detects cross compilation but so far I haven't been able to understand the internal logic all that well.

Would you happen to know if there's a good place to find someone familiar with those internals enough to give me some pointers to help me better understand how the auto-detection there currently works?

Best I can tell none of the longstanding github issues relating to this issue that were opened against the cargo or rust repos have been given any serious attention by anyone who has a good understanding of what's going on internally so far.

@davidhewitt
Copy link
Member

You could try the t-cargo stream on the Rust zulip server?

@jameshilliard
Copy link
Contributor Author

For reference I have an open pull request to fix the related cargo bug in #9322.

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.

3 participants