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

refactored rust_toolchain into rust_exec_toolchain and rust_target_toolchain #800

Closed
wants to merge 2 commits into from

Conversation

@google-cla google-cla bot added the cla: yes label Jun 25, 2021
@UebelAndre
Copy link
Collaborator Author

Just so everyone's aware of where all the PRs came from. I've been working on refactoring the toolchains so it's easier to register target toolchains for cross-compilation. There's still some rough edges here but the gist of this is the previous rust_toolchain has been split into exec and target variants where exec contains the compiler and things and target contains all the linkable libraries for the target platform. This is largely inspired by #523 and a conversation that was had in slack. As the other PRs get closed out, I'll be making minor changes here and adding documentation.

@UebelAndre UebelAndre force-pushed the cross branch 27 times, most recently from b09207a to a33dc69 Compare June 27, 2021 17:40
@hlopko
Copy link
Member

hlopko commented Jun 28, 2021

I'm concerned that the approach you're taking may not be the best possible. Could you first start a discussion about:

  • what you're trying to achieve?
  • why is it important?
  • how do you want to achieve it?

Whenever there is a big "chunk" of work that requires so many changes we should get into a habit of discussing the general approach first, and only starting coding afterwards. It's very frustrating to the author to receive substantial feedback in the middle of the "project" that invalidates prior work. Communicating up front makes it easier for the reviewers to understand the context and motivation behind changes.

@UebelAndre
Copy link
Collaborator Author

Closing this based on #815 (comment)

@UebelAndre UebelAndre closed this Jan 27, 2022
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