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

Update formatting of --extern flags passed to rustc #892

Merged
merged 3 commits into from
Aug 24, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Aug 14, 2021

In an effort to make the the rules more testable, the --extern flags, which were previously:

['--extern', 'crate=some/path/to/libcrate.rlib']

have been updated to:

['--extern=crate=some/path/to/libcrate.rlib']

Additionally, a test has been added to test regressions in accessibility to symbols in dependencies and transitive dependencies of a target. re-exports (ie pub use ...) for example.

Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

I think I'd expect a test that checks that we pass all transitive dependencies as -L and as --extern flags. If we didn't do that, your test would fail even if it didn't use pub use. In that sense mentioning re-exporting is a distraction, a red-herring, that is only slightly related to the test at hand. The fact that there is re-exporting happening is only known to rustc internally, and rules_rust don't know about it (and they don't need to know about it because of how rustc compilation model works).

What do you think about renaming the test to something like transitive deps flags test?

What do you think

test/unit/exports/lib_b/src/lib.rs Show resolved Hide resolved
@UebelAndre
Copy link
Collaborator Author

I think I'd expect a test that checks that we pass all transitive dependencies as -L and as --extern flags. If we didn't do that, your test would fail even if it didn't use pub use. In that sense mentioning re-exporting is a distraction, a red-herring, that is only slightly related to the test at hand. The fact that there is re-exporting happening is only known to rustc internally, and rules_rust don't know about it (and they don't need to know about it because of how rustc compilation model works).

What do you think about renaming the test to something like transitive deps flags test?

What do you think

I only care that there's a test that checks this specific functionality since it's the behavior I care about. If the unittest code should actually be checking different flags then that's fine, but I'd run into an issue where it was believed this functionality was not supported so I'm hoping for something that very clearly states it is. Does that change the ask here?

@UebelAndre UebelAndre requested a review from hlopko August 19, 2021 14:37
rust/private/rustc.bzl Show resolved Hide resolved
@UebelAndre UebelAndre changed the title Added test for re-exported symbols Update formatting of --export flags passed to rustc Aug 24, 2021
@hlopko
Copy link
Member

hlopko commented Aug 24, 2021

I don't know the details of the discussion you were in, but can it be that the person unaware of this already working was rather new to Rust/Bazel/rules_rust? For those people adding an unit test is not the best way to show how Rustc (not rules_rust, rustc) handles reexports. Maybe we need a text file documenting things like this in a approachable way.

For everybody else, the unit test with reexports distracts from the actual thing the test is testing.

@UebelAndre UebelAndre changed the title Update formatting of --export flags passed to rustc Update formatting of --extern flags passed to rustc Aug 24, 2021
@UebelAndre UebelAndre merged commit a366b76 into bazelbuild:main Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants