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.

Sign up for GitHub

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

Jump to bottom

Allow deps to be renamed #285

Merged
merged 2 commits into from
Jan 29, 2020
Merged

Allow deps to be renamed #285

merged 2 commits into from
Jan 29, 2020

Conversation

GregBowyer
Copy link
Contributor

No description provided.

@GregBowyer GregBowyer mentioned this pull request Dec 20, 2019
Closed
damienmg
damienmg reviewed Dec 20, 2019
View reviewed changes
Copy link
Collaborator

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

Hi, Thanks for sending out this PR.

What is the goal of the alias?

It seems to me one of the goal would be to avoid crate name collision but this PR does not address this case.

Also could you had an aliasing test case in the examples?

rust/private/rust.bzl Outdated
@@ -275,6 +279,13 @@ _rust_common_attrs = {
linking a native library.
"""),
),
"aliases": attr.label_keyed_string_dict(
doc = _tidy("""
Remap crates to a new name or monikor for linkage to this target
Copy link
Collaborator

Choose a reason for hiding this comment

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

monikor is a typo?, of monitor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo, should be moniker

rust/private/rust.bzl Outdated
doc = _tidy("""
Remap crates to a new name or monikor for linkage to this target

These are other `rust_library` targets and will be presented as the new name given
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfinished sentence.

@GregBowyer
Copy link
Contributor Author

Hi, Thanks for sending out this PR.

What is the goal of the alias?

It seems to me one of the goal would be to avoid crate name collision but this PR does not address this case.

Also could you had an aliasing test case in the examples?

Sorry I dashed this out a little quickly yesterday to pack up just before the holidays.

This is to allow for crate style renames, this is something that occurs very occasionally in crates, the basic idea is to present to a library an external crate under a different name.

The big one where this showed up was in compiling futures at version 0.3.x, with the feature compat. This leads to a situation where futures-util-0.3.x (a dependency of futures as a bill of materials) claims a dep in Cargo.toml like so.

[dependencies.futures_01]
version = "0.1.25"
optional = true
package = "futures"

That is, it depends on an earlier version of futures and includes it, from its viewpoint, as the crate futures_01

This rename is achieved through changing the name on the --extern parameter passed to the compiler.

This is related to google/cargo-raze#123

Without this, its pretty hard to compile crates that perform renames without patching

damienmg
damienmg approved these changes Jan 2, 2020
View reviewed changes
Copy link
Collaborator

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

Just need to address the two comments on comments.

@GregBowyer
Copy link
Contributor Author

Addressed the comments and added some basic tests

@kornholi kornholi mentioned this pull request Jan 8, 2020
Open
damienmg
damienmg approved these changes Jan 9, 2020
View reviewed changes
Copy link
Collaborator

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

There are failure on the CI, could you address them?

@GregBowyer
Copy link
Contributor Author

Looks like the build error was an unrelated stardoc incompatibility change, I will push that as a seperate PR that this is based on

@GregBowyer
Copy link
Contributor Author

GregBowyer commented Jan 10, 2020 •
edited
Loading

This currently is blocked / deps on #290

@GregBowyer
Copy link
Contributor Author

Rebasing fun, do we want to merge this to unlock cargo-raze?

@damienmg damienmg merged commit a1bfa0d into bazelbuild:master Jan 29, 2020
@damienmg
Copy link
Collaborator

Thanks for this PR!

@GregBowyer GregBowyer deleted the alias-deps branch January 30, 2020 01:17
@ipetr0v ipetr0v mentioned this pull request Apr 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damienmg damienmg damienmg approved these changes

@acmcarther acmcarther Awaiting requested review from acmcarther

@mfarrugi mfarrugi Awaiting requested review from mfarrugi

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GregBowyer @damienmg @googlebot