Skip to content

Conversation

@dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Apr 24, 2022

I found half-baked duplicate implementations of this logic in cargo-raze (examples_repository.bzl), in cxx (vendor.bzl), and in rules_rust's test suite of the load_arbitrary_tool function (load_arbitrary_tool_test.bzl). It seems such logic is necessary for correctly using load_arbitrary_tool from the implementation of a repository_rule.

Documentation

Example:

load("@rules_rust//rust:repositories.bzl", "load_arbitrary_tool")
load("@rules_rust//rust/platform:triple.bzl", "get_host_triple")
 
def _impl(repository_ctx):
    load_arbitrary_tool(
        ctx = repository_ctx,
        tool_name = "cargo",
        tool_subdirectories = ["cargo"],
        target_triple = get_host_triple(repository_ctx).str,
    )
 
example = repository_rule(implementation = _impl)

Args:

  • repository_ctx (repository_ctx): The repository_rule's context object
  • abi (str): Since there's no consistent way to check for ABI, this info may be explicitly provided

Returns:

  • struct: A triple struct; see the triple function in this module

@dtolnay dtolnay changed the title Add target_triple_for_os to convert Bazel's repository_ctx.os to target triple Make get_host_triple public to get a triple from Bazel's repository_ctx Apr 25, 2022
version = "1.53.0",
iso_date = None,
target_triple = target_triple,
target_triple = host_triple.str,
Copy link
Contributor Author

@dtolnay dtolnay Apr 25, 2022

Choose a reason for hiding this comment

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

I added a str field to the struct returned by triple("..."), because triple.triple didn't strike me as evocative enough. str makes sense to me as the string representation of the triple, which is what target_triple wants.

Alternatively we could look into making target_triple accept the triple struct directly, or union of triple struct and string so that either way works.

Or we could make get_host_triple return the string and not the struct, forcing triple(get_host_triple(repository_ctx)) when someone needs the struct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a str field to the struct returned by triple("..."), because triple.triple didn't strike me as evocative enough. str makes sense to me as the string representation of the triple, which is what target_triple wants.

I'm fine with the use of str but I'd prefer there to be either str or triple. Could you maybe update the docstring to indicate which of the two is the correct to use?

I added a str field to the struct returned by triple("..."), because triple.triple didn't strike me as evocative enough. str makes sense to me as the string representation of the triple, which is what target_triple wants.

Alternatively we could look into making target_triple accept the triple struct directly, or union of triple struct and string so that either way works.

Or we could make get_host_triple return the string and not the struct, forcing triple(get_host_triple(repository_ctx)) when someone needs the struct.

I think I would prefer to continue to pass the triple struct until we find a performance reason to do otherwise. We have a lot of places where we parse the same string over and over. I expect passing a struct to be slightly better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

         - str (str): Original string representation of the triple
-        - triple (str): Same as str
+        - triple (str): Deprecated; same as str

I'll open another PR after this to remove triple so that we can discuss that independently of this change.

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

If you could update one comment and rebase we can get this in 😄

version = "1.53.0",
iso_date = None,
target_triple = target_triple,
target_triple = host_triple.str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a str field to the struct returned by triple("..."), because triple.triple didn't strike me as evocative enough. str makes sense to me as the string representation of the triple, which is what target_triple wants.

I'm fine with the use of str but I'd prefer there to be either str or triple. Could you maybe update the docstring to indicate which of the two is the correct to use?

I added a str field to the struct returned by triple("..."), because triple.triple didn't strike me as evocative enough. str makes sense to me as the string representation of the triple, which is what target_triple wants.

Alternatively we could look into making target_triple accept the triple struct directly, or union of triple struct and string so that either way works.

Or we could make get_host_triple return the string and not the struct, forcing triple(get_host_triple(repository_ctx)) when someone needs the struct.

I think I would prefer to continue to pass the triple struct until we find a performance reason to do otherwise. We have a lot of places where we parse the same string over and over. I expect passing a struct to be slightly better.

I found half-baked duplicate implementations of this logic in
cargo-raze, in cxx, and in rules_rust's test suite of the
load_arbitrary_tool function. It seems such logic is necessary for
correctly using load_arbitrary_tool from the implementation of a
repository_rule.
@dtolnay
Copy link
Contributor Author

dtolnay commented Apr 26, 2022

Rebased and updated the documentation of the return type of triple("..."):

         - str (str): Original string representation of the triple
-        - triple (str): Same as str
+        - triple (str): Deprecated; same as str

I'll open another PR after this to remove triple from this struct so that we can discuss that independently of this change.

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.

3 participants