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

ScalaPB deps toolchain #1104

Merged
merged 7 commits into from
Oct 21, 2020
Merged

ScalaPB deps toolchain #1104

merged 7 commits into from
Oct 21, 2020

Conversation

liucijus
Copy link
Collaborator

Refactors scalapb toolchain to use DepsInfo providers to pass required dependencies. Part of #940

@liucijus liucijus requested a review from ittaiz as a code owner September 11, 2020 08:46
@ittaiz
Copy link
Member

ittaiz commented Sep 11, 2020

is this a breaking change?

@liucijus
Copy link
Collaborator Author

is this a breaking change?

If I'm not missing anything, it should not be breaking (it refactors private attributes)

@ittaiz
Copy link
Member

ittaiz commented Sep 11, 2020

Build is broken

@liucijus
Copy link
Collaborator Author

@ianoc-stripe do you remember why this test is needed? Toolchains automatically include host paltform as an execution platform. Is it a problem?

@liucijus liucijus marked this pull request as draft September 17, 2020 07:38
@liucijus
Copy link
Collaborator Author

Moving this PR to draft, due to:
Generators like @io_bazel_rules_scala//src/scala/scripts:scalapb_worker depend on the same libs that come via proto toolchain, which ends up in a cyclic resolution here. I assume we need to introduce another toolchain for proto deps, so generators can get deps before their are resolved from the proto toolchain.

@liucijus liucijus mentioned this pull request Sep 18, 2020
Closed
@liucijus
Copy link
Collaborator Author

We may need to upgrade to at least Bazel 3.5.0 address toolchain limitations (#801 ). @ittaiz WDYT?

@ittaiz
Copy link
Member

ittaiz commented Sep 19, 2020

I can live with that if I understand the exact value gain and reason.
Value gain is #751 ?
If so why do we need 3.5.0 for exactly? What does toolchain limitations mean? Best if we can have a text and a link to bazel issue + PR

@liucijus
Copy link
Collaborator Author

Limitation is that with a current state of toolchains, all deps are added as host: #797. It's address with an ongoing work on execution transitions (https://groups.google.com/g/bazel-discuss/c/vXrG6uFL5V8/m/RjK8_chaEgAJ).
Even with 3.5.0 it is problematic as it requires flag --incompatible_override_toolchain_transition to be used. It would be reasonable to wait until the full support in later Bazel versions, but then we need an alternative way to address #751

@ianoc-stripe
Copy link
Contributor

@liucijus I might be missing something but how does the deps coming via the bind or the toolchain effect #751 ?

@liucijus
Copy link
Collaborator Author

@liucijus I might be missing something but how does the deps coming via the bind or the toolchain effect #751 ?

Bind should be ok, but toolchain deps will be added as host deps, no?

@ianoc-stripe
Copy link
Contributor

@liucijus having them in the bind keeps it off the host deps. But the issues around having deps in the toolchain or not is unconnected with #751 as far as i know. You can add/change the deps via the bind (indeed we bind these against local java_libraries iirc to setup the deps we use locally). It would be nice to move the deps on the toolchain since it seems like thats how it should be done, but afaik it won't solve any of our other issues today

@liucijus liucijus mentioned this pull request Oct 15, 2020
Merged
Vaidas Pilkauskas added 7 commits October 16, 2020 13:14
Inject Scalapb deps via providers on the toolchain
5041868
Update scalapb toolchain deps docs
86b63cf
Lint
3b66e89
Use external names for default labels
258a02a
Add proto toolchain only for deps
f9a18bf 
`//scala_proto:deps_toolchain_type` is required to uncycle dependency between `//scala_proto:toolchain_type` and generators which depend on the same proto libraries and need to be initialized before the toolchain creation.
Update toolchain transitions
7aa5c17
Use incompatible_use_toolchain_transition
5e51126
@liucijus liucijus marked this pull request as ready for review October 19, 2020 08:15
@liucijus
Copy link
Collaborator Author

this PR is ready to be merged

@ittaiz ittaiz merged commit e3c2382 into bazelbuild:master Oct 21, 2020
blorente pushed a commit to twitter-forks/rules_scala that referenced this pull request Nov 26, 2020
ScalaPB deps toolchain (bazelbuild#1104)
5333ee3 
* Inject Scalapb deps via providers on the toolchain

* Update scalapb toolchain deps docs

* Lint

* Use external names for default labels

* Add proto toolchain only for deps

`//scala_proto:deps_toolchain_type` is required to uncycle dependency between `//scala_proto:toolchain_type` and generators which depend on the same proto libraries and need to be initialized before the toolchain creation.

* Update toolchain transitions

* Use incompatible_use_toolchain_transition
@liucijus liucijus mentioned this pull request Dec 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ittaiz ittaiz ittaiz approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@liucijus @ittaiz @ianoc-stripe @googlebot