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

Added module for abi constraint values #39

Closed
wants to merge 3 commits into from

Conversation

UebelAndre
Copy link

This pull-requests adds constraint_value flags for common abi flags. Defining these in the @platforms repository avoids other rules from needing to define custom constraints and/or avoids each repository defining project specific constraints.

closes #38

abi/BUILD Outdated Show resolved Hide resolved
abi/BUILD Outdated
)

constraint_value(
name = "linuxkernel",

Choose a reason for hiding this comment

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

This doesn't seem like it belongs? Kernel code can be built with various other options from this list, there's nothing particularly special about object files built for the kernel.

Copy link
Author

Choose a reason for hiding this comment

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

Fair. This was just a value listed in https://doc.rust-lang.org/nightly/rustc/platform-support.html#tier-3 so it was included in what I added. As per most of my comments on this PR, I'm happy to remove this and have a more focused conversation on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think using Rust, or Go, or linux kernel builds, or whatever as the pattern to emulate wins.

Was there a design proposal for this? It is much easier to discuss in a doc than in PR review.

Copy link
Author

Choose a reason for hiding this comment

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

Aside from this there's just the github issue (#38). I tried emailing //bazel-dev@googlegroups.com per the README but the address no longer seems valid.

@bsilver8192
Copy link

gnueabihf commonly means all of these, of the ones written:

  • eabi
  • eabihf
  • elf
  • gnu
  • gnu_ilp32 (all 3 types 32 bits, but this term is usually only applied to 64-bit instruction sets, so maybe not)
  • gnueabi
  • gnueabihf

Some of these might be meant as opposites of each other, but that's unclear. Even if it was written in a comment, I suspect people will end up picking the wrong ones via copy/paste a lot.

Also, AFAIK a platform can only have a single of these constraint_values, which doesn't seem like it works very well for these.

I don't have a good proposal here, but I think this will be confusing and worse than the status quo.

@UebelAndre
Copy link
Author

@bsilver8192 thanks so much for taking a look! The values here were all taken from platform triples listed on https://doc.rust-lang.org/nightly/rustc/platform-support.html

While it'd be nice to represent everything there, I'd be fine with reducing this PR to common and less controversial values. Maybe just the following for starters and have deeper conversations about more unique values?:

  • gnu
  • musl
  • msvc

@bsilver8192
Copy link

While it'd be nice to represent everything there, I'd be fine with reducing this PR to common and less controversial values. Maybe just the following for starters and have deeper conversations about more unique values?:

  • gnu
  • musl
  • msvc

What semantics are you proposing for each one? Those strings in the ABI part of a triple mean different things depending on the rest of the triple, so I don't think we can define them that way. Leaving it up to Bazel users to come up with their own meanings seems counterproductive, because you end up with conflicts when trying to use rule ecosystems together. Also, there are lots of places that use "target triples" with subtly different, conflicting meanings and sets of choices for each component, for example:

@UebelAndre
Copy link
Author

What semantics are you proposing for each one? Those strings in the ABI part of a triple mean different things depending on the rest of the triple, so I don't think we can define them that way. Leaving it up to Bazel users to come up with their own meanings seems counterproductive, because you end up with conflicts when trying to use rule ecosystems together. Also, there are lots of places that use "target triples" with subtly different, conflicting meanings and sets of choices for each component, for example:

* Clang

* Rust (not the same as Clang, https://github.com/rust-lang/rust-bindgen/blob/c27578fac5372575e3caa933a83eac931ae3b53f/src/lib.rs#L2233 has some differences)

* GCC

* Multiarch

That's a great question. I personally don't have a super strong opinion, for Rust I think rules_rust could probably provide some aliases to provide more familiar constraints and would expect other rules/languages to do the same. the @platforms repository should have the simplest and most common values to really benefit the larger community here.

@aiuto aiuto requested review from katre and aiuto April 11, 2022 01:24
@gregestren gregestren self-assigned this Apr 11, 2022
@aiuto
Copy link
Contributor

aiuto commented Oct 4, 2022

This has lingered too long.
I think we should not merge this as is, but take a step back and wait for a proposal that talks about what need a new abi space is serving problems, and shows how it will be applied to multiple languages (not just rust) and oses. In particular, I would like to see the edges, like cross compilation and building for mixed C++, Go & rust applications.

@aiuto aiuto added the wontfix This will not be worked on label Oct 4, 2022
@UebelAndre UebelAndre closed this Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constraints for ABI
4 participants