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

Remove the legacy .proto. provider #6901

Closed
lberki opened this issue Dec 12, 2018 · 12 comments
Closed

Remove the legacy .proto. provider #6901

lberki opened this issue Dec 12, 2018 · 12 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-CPP Issues for C++ rules type: process

Comments

@lberki
Copy link
Contributor

lberki commented Dec 12, 2018

We now have the ProtoInfo provider, which is its modern version.

A flag needs to be added that disables ctx.dep.proto. and all uses need to be migrated to ctx.dep[ProtoInfo].

@lberki lberki added P2 We'll consider working on this in future. (Assignee optional) team-Starlark labels Dec 12, 2018
@ittaiz
Copy link
Member

ittaiz commented Dec 13, 2018 via email

@lberki
Copy link
Contributor Author

lberki commented Dec 13, 2018

What's the sources provider? (ProtoSourcesProvider got renamed to ProtoInfo, but it's a class name that should be internal to Bazel)

@ittaiz
Copy link
Member

ittaiz commented Dec 13, 2018 via email

@lberki
Copy link
Contributor Author

lberki commented Dec 13, 2018

[ProtoInfo] is the same provider as .proto indeed. It's even the same implementation and all. Mind you, it's somewhat broken and we should totally fix it some day, but it's broken in the same way as .proto is broken so the migration should consist solely of replacing .proto with [ProtoInfo].

bazel-io pushed a commit that referenced this issue Dec 13, 2018
Also fix an embarrassing issue that made the tests in BazelProtoInfoStarlarkTest silently *always pass*. The root cause of that is that we swallow loading-phase errors for some reason (#6902), but let's fix that in a different change.

Progress towards #6901.

RELNOTES: None.
PiperOrigin-RevId: 225339799
@laurentlb
Copy link
Contributor

Related to #7347

I don't think we should special case .proto. We should have a flag to forbid all the old style providers.

@lberki
Copy link
Contributor Author

lberki commented Apr 8, 2019

If you are willing to take a step that's that big...

@Bocete
Copy link
Contributor

Bocete commented Jul 3, 2019

Are there plans to make ProtoInfo instantiable by users? If so, is there an ETA? Currently, the ProfoInfo constructor isn't callable.

I understand that ProtoInfo has some legacy muck that would be a bit dangerous if exposed to users. But perhaps it could be made work when specifying non-legacy fields only? Maybe that's possible, maybe it isn't; I'm not familiar with the inner workings of that provider.

Thanks! I'd love to contribute to make this happen if you think it's the right thing to do, and you're only strapped for cycles.

@hlopko
Copy link
Member

hlopko commented Jul 4, 2019

Why do you need to instantiate the provider? That would typically make sense when creating your own iproto_library, and in that case I'd prefer to improve existing proto_library.

@hlopko hlopko assigned dbabkin and unassigned c-parsons Jul 4, 2019
@hlopko
Copy link
Member

hlopko commented Jul 4, 2019

Ah now I see your comment on protocolbuffers/protobuf#6163 (comment). That's a very complicated usecase that I don't understand :)

You write you "suspect" that this will help you, or maybe the Proto sandwich would. I think of sandwich API as not a way to call native.proto_library, but a way to register the same actions as proto_library would. Can you maybe simplify the usecase so I can understand it and maybe we'll come up with a way forward?

@dslomov dslomov removed the bazel 1.0 label Jul 24, 2019
@dbabkin dbabkin removed their assignment Oct 11, 2019
@brandjon
Copy link
Member

I don't think we should special case .proto. We should have a flag to forbid all the old style providers.

If you are willing to take a step that's that big...

I'd love to remove legacy struct providers this year, not sure whether it'll happen. In the meantime, if a flag helps the proto rules migrate off legacy structs, that's fine; I took the same approach in --incompatible_disallow_legacy_py_provider.

Triaging to the Build API team.

@brandjon brandjon added team-Rules-CPP Issues for C++ rules untriaged and removed P2 We'll consider working on this in future. (Assignee optional) team-Starlark labels Feb 16, 2021
@oquenchil oquenchil added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: process and removed untriaged labels May 10, 2021
@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label May 10, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-CPP Issues for C++ rules type: process
Projects
None yet
Development

No branches or pull requests

10 participants