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

Investigate incorrect target detection #1376

Merged
merged 3 commits into from
Sep 23, 2023
Merged

Investigate incorrect target detection #1376

merged 3 commits into from
Sep 23, 2023

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Sep 21, 2023

See #1375.

@tamird
Copy link
Contributor Author

tamird commented Sep 21, 2023

@NobodyXu

tamird added a commit to aya-rs/book that referenced this pull request Sep 21, 2023
This should fix CI. See
cargo-bins/cargo-binstall#1376 and
https://github.com/cargo-bins/cargo-binstall/issue/1375 - it seems that
cargo-binstall doesn't work correctly on ubuntu-20.04.
@tamird
Copy link
Contributor Author

tamird commented Sep 21, 2023

@NobodyXu need another run please; I've moved the logs to stderr so they don't interfere with the shell script.

crates/detect-targets/src/detect/linux.rs Outdated Show resolved Hide resolved
String::from_utf8_lossy(&stdout)
.contains("GLIBC")
.then_some(Libc::Gnu)
let stdout = String::from_utf8_lossy(&stdout);
Copy link
Member

Choose a reason for hiding this comment

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

The bug resides in here, from the log:

while loading shared libraries: --version: cannot open shared object file\n")

My guess is that on ubuntu-20.04, glibc isn't executable.

To fix this, we can simply check that one of these files exist instead of executing them.

Or we can also check os-release but that is too complex.

tokio = { version = "1.28.2", features = ["rt", "process", "sync"], default-features = false }
tracing = "0.1.37"
tracing-subscriber = { version = "0.3.17", features = ["fmt"], default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

You can make tracing-subscriber an optional dependency, only enabled if the binary is built.

crates/detect-targets/Cargo.toml Outdated Show resolved Hide resolved
@tamird
Copy link
Contributor Author

tamird commented Sep 21, 2023

The intent of this PR is to demonstrate the problem. I've removed the futures_util dependency. I'm not sure how to make tracing-subscriber optional in the way you describe. Please feel free to update this PR or create a new one using the code in this one.

@NobodyXu
Copy link
Member

The intent of this PR is to demonstrate the problem. I've removed the futures_util dependency. I'm not sure how to make tracing-subscriber optional in the way you describe. Please feel free to update this PR or create a new one using the code in this one.

Please feel free to continue using the existing code since this PR is mostly for demonstrating the bug and finding the fix.

I was just being pedantic.

@NobodyXu
Copy link
Member

Can I close this PR @tamird ?

@tamird
Copy link
Contributor Author

tamird commented Sep 23, 2023

I think the debug logs might still be useful. I can rebase if you agree.

@NobodyXu
Copy link
Member

That's true, I can do the rebase for you and apply the suggestion I wanted.

@tamird
Copy link
Contributor Author

tamird commented Sep 23, 2023

Sounds good; go ahead!

@NobodyXu NobodyXu added this pull request to the merge queue Sep 23, 2023
Merged via the queue into cargo-bins:main with commit 2db8e25 Sep 23, 2023
29 checks passed
@NobodyXu
Copy link
Member

Thanks @tamird !

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