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 cargo-xwin build by using CARGO_CFG_TARGET_OS env var instead of #[cfg(target_os = "x") #96

Merged
merged 3 commits into from
May 30, 2023

Conversation

dzervas
Copy link
Contributor

@dzervas dzervas commented May 26, 2023

To my surprise something is wrong with the #[cfg(target_os = "whatever")]. My journey started when I tried to build my project that uses the frida crate using cargo-xwin - I wanted to cross-compile it from a linux host to target x86_64-pc-windows-msvc. When I did that the linker nagged that it couldn't find pthread.lib and resolv.lib - which are not windows libraries. They shouldn't be required. The problem was that for some reason the target_os value was not set according to the target but rather according to the host

I THINK it's due to the build.rs being targeted to the host (as it's the build script).

Some useful links:

If this gets merged can we get a new patch release please?

s1341
s1341 previously approved these changes May 28, 2023
Copy link
Contributor

@s1341 s1341 left a comment

Choose a reason for hiding this comment

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

LGTM.

@meme
Copy link
Collaborator

meme commented May 28, 2023

Likewise, but I wonder what happened to the checks here... can you maybe push another commit to re-run the CI?

meme
meme previously approved these changes May 28, 2023
Copy link
Collaborator

@meme meme left a comment

Choose a reason for hiding this comment

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

Thanks, great catch.

@dzervas
Copy link
Contributor Author

dzervas commented May 28, 2023

Likewise, but I wonder what happened to the checks here... can you maybe push another commit to re-run the CI?

Problem is it 404s in some packages. Me committing somethings won't change anything, as I see that you tried to re-run the jobs

Maybe something's wrong?

@s1341
Copy link
Contributor

s1341 commented May 29, 2023

I've fixed CI in #97. Once it's merged, you can rebase and we can merge this.

@meme
Copy link
Collaborator

meme commented May 29, 2023

If you rebase CI should be passing. Thanks.

@dzervas dzervas dismissed stale reviews from meme and s1341 via bee60c6 May 29, 2023 22:27
@dzervas
Copy link
Contributor Author

dzervas commented May 29, 2023

just fixed the formatting

@meme meme merged commit 6be3238 into frida:main May 30, 2023
14 checks passed
@s1341
Copy link
Contributor

s1341 commented Jun 1, 2023

@meme can you please cut a version with this change?

@meme
Copy link
Collaborator

meme commented Jun 1, 2023

Done!

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

3 participants