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 targets with zig toolchain #986

Closed
wants to merge 1 commit into from

Conversation

QuentinPerez
Copy link

When you use bazel + zig to cross compile a rust codebase, you got an 'invalid target error'
To fix the issue, a check has been added to know if it's a zig toolchain, not sure it's the right way to do it.
Feel free to take over the PR.

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

It would be great if you could setup CI for testing zig-cc, to verify it indeed works as intended.

@QuentinPerez
Copy link
Author

It would be great if you could setup CI for testing zig-cc, to verify it indeed works as intended.

I tried it, but I am not sure how to achieve it correctly. Do you have any ideas or guidance on how I could do it?

@NobodyXu
Copy link
Collaborator

NobodyXu commented Mar 3, 2024

Simply install pip3 install zig in job test of .github/workflows/main.yml.

You would also need to add a tests/zig-test.rs and write all the testing for zig-cc in it.

src/tool.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Collaborator

@QuentinPerez #1000 has been merged, can you rebase against main please?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, the code itself LGTM, can you please add a test for zig-cc in CI?

You can just run cargo-test, while setting environment variable CC to zig-cc

@NobodyXu
Copy link
Collaborator

Discussed this with @thomcc on zulip, he mentioned that the zig-cc target triple might be changed in the later version of zig-cc.

We are afraid that after merging this PR, zig-cc might change its target triple and break this, causing more code to require to support both newer version and older version.

We decided to hold off adding more workarounds for zig-cc until it reaches 1.0, when its target triple and cmdline args are stable.

@NobodyXu NobodyXu closed this Mar 29, 2024
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.

2 participants