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 some if cfg!(unix) mis-usages #708

Closed
kkysen opened this issue Oct 18, 2022 · 3 comments · Fixed by #1036
Closed

Fix some if cfg!(unix) mis-usages #708

kkysen opened this issue Oct 18, 2022 · 3 comments · Fixed by #1036
Assignees
Labels
good first issue Good for newcomers

Comments

@kkysen
Copy link
Contributor

kkysen commented Oct 18, 2022

See #519 (comment) by @SimonIT.

I used if cfg!(unix) wrong (instead of #[cfg(unix)]), so some simple snippets don't compile on non-unix platforms anymore.

This is in c2rust-build-paths/src/lib.rs and dynamic_instrumentation/src/main.rs.

@kkysen kkysen self-assigned this Oct 18, 2022
@kkysen kkysen added enhancement New feature or request good first issue Good for newcomers and removed enhancement New feature or request labels Jan 4, 2023
kkysen added a commit that referenced this issue Nov 1, 2023
kkysen added a commit that referenced this issue Nov 3, 2023
kkysen added a commit that referenced this issue Nov 3, 2023
* Fixes #709.
* Fixes #708.

This adds an MVP of a `cargo` wrapper for `c2rust-analyze` so that it
can be run on a whole crate like `cargo`. This copies the approach from
`c2rust-instrument`, with some minor adjustments:
* We don't care about the instrumentation and metadata
`c2rust-instrument` need, so that code is gone.
* We still allow `c2rust-analyze` to be called as a `rustc_wrapper`
directly. More specifically, the `cargo` wrapper is supposed to set
`RUST_SYSROOT`. `c2rust-instrument` requires this, while
`c2rust-analyze` will re-calculate it if it wasn't.

This allows us to keep using the `rustc` wrapper in tests,
as the tests are all set up as single files meant to be compiled with
`rustc` directly.
We can change this, but that'll come later. The exception is the
`lighttpd_minimal` and `with_pdg_file` tests, as those run on full
crates. I converted `lighttpd_minimal` to use the `cargo` wrapper as a
proof of concept to show it works. I tried to do the same for
`with_pdg_file`, but it updated some of the hashes or something so now
the PDG binary has out-of-date `DefPathHash`es, and I'm not sure how to
regenerate a new one.
@MurraySobol
Copy link

MurraySobol commented Nov 3, 2023

Thanks for fixing this particular Windows error, I understand that there are (probably) several further Windows 11 errors to be fixed at some time.
Baby steps before giant leaps!!
BTW: I just reran "cargo install c2rust" and got a similar, but I think a slightly different error:
Compiling c2rust-build-paths v0.18.0
error[E0433]: failed to resolve: could not find unix in os
--> C:\Users\murra.cargo\registry\src\index.crates.io-6f17d22bba15001f\c2rust-build-paths-0.18.0\src\lib.rs:35:26
|
35 | use std::os::unix::ffi::OsStrExt;
| ^^^^ could not find unix in os

error[E0599]: no function or associated item named from_bytes found for struct OsStr in the current scope
--> C:\Users\murra.cargo\registry\src\index.crates.io-6f17d22bba15001f\c2rust-build-paths-0.18.0\src\lib.rs:37:20
|
37 | OsStr::from_bytes(path)
| ^^^^^^^^^^ function or associated item not found in OsStr

I noted that it was trying to install this:
C:>rustup update
info: syncing channel updates for 'stable-x86_64-pc-windows-msvc'
info: syncing channel updates for 'nightly-x86_64-pc-windows-msvc'
info: checking for self-update

stable-x86_64-pc-windows-msvc unchanged - rustc 1.73.0 (cc66ad468 2023-10-03)
nightly-x86_64-pc-windows-msvc unchanged - rustc 1.75.0-nightly (a2f5f9691 2023-11-02)

info: cleaning up downloads & tmp directories

I assume that this means that I should wait for the next official update of Rust, since this change wouldn't be merged into the current build?

Thanks again for all of your efforts.

@kkysen
Copy link
Contributor Author

kkysen commented Nov 3, 2023

Hi! It doesn't have anything to do with the version of rust itself, but you'd have to wait for the next release of c2rust to be able to cargo install c2rust the newest version. But you can cargo install --git https://github.com/immunant/c2rust/ and it'll build the latest master version, which includes the above fix.

@MurraySobol
Copy link

MurraySobol commented Nov 3, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants