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 objc_proto_library rule #7348

Closed
laurentlb opened this issue Feb 4, 2019 · 13 comments
Closed

Remove objc_proto_library rule #7348

laurentlb opened this issue Feb 4, 2019 · 13 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-Server Issues for serverside rules included with Bazel type: process

Comments

@laurentlb
Copy link
Contributor

objc_proto_library is not usable in Bazel. The code is present because it may be used inside Google. We should remove the rule from Bazel and from its documentation (https://docs.bazel.build/versions/master/be/objective-c.html#objc_proto_library).

(tracking bug in Google: b/123888674)

@laurentlb laurentlb added P1 I'll work on this now. (Assignee required) type: process z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple bazel 1.0 labels Feb 4, 2019
@lberki
Copy link
Contributor

lberki commented Feb 5, 2019

@lberki
Copy link
Contributor

lberki commented Feb 5, 2019

What about e.g. apple_binary -> proto_library dependencies?

Removing the rule itself is a trivial amount of work, but it appears that apple_binary and apple_static_library also use the aspect. Does this mean that those rules can't depend directly on proto_library rules?

@aragos
Copy link
Contributor

aragos commented Feb 5, 2019

apple_binary cannot directly depend on proto_library targets. It does however register an aspect on its transitive dependencies to aid processing of any objc_proto_library target it ends up linking. We'll have to decide whether to remove this code as well (it does nothing without the presence of objc_proto_library) or to leave it in place.

@lberki
Copy link
Contributor

lberki commented Feb 5, 2019

What does that aspect do then? I mean, objc_proto_library propagates that aspect just as well, so what extra functionality does apple_binary / apple_static_library gain from it?

@sergiocampama
Copy link
Contributor

apple_binary propagates the aspect to collect all proto_library sources and other files to generate a comprehensive list of protos and which symbols are required, and at that point generate/compile/link them into the binary.

We'd like to remove the processing done in apple_binary at some point, but disallowing the objc_proto_library rule from Bazel should be orthogonal to that cleanup.

Please ping me for a more in depth description of how objc protos work, if you want to gaze into the abyss.

@lberki
Copy link
Contributor

lberki commented Feb 6, 2019

Well, I thought it was simpler than that :) My motivation was to also remove the aspect implementation and not just objc_proto_library itself.

Let's continue the discussion on the "Bazel 1.0 and objc_proto_library" thread.

@dnkoutso
Copy link

Related #1802

@dslomov
Copy link
Contributor

dslomov commented Jul 24, 2019

This P1 bazel 1.0 issue is unassigned. Please assign cc @aragos

@thomasvl
Copy link
Member

@sergiocampama is working on this

@dslomov
Copy link
Contributor

dslomov commented Sep 2, 2019

Does not seem to be on track for 1.0

@dslomov dslomov removed the bazel 1.0 label Sep 2, 2019
@sergiocampama
Copy link
Contributor

objc_proto_library has been deleted from the native codebase, but not all native support has been removed. We would have liked to remove this sooner, but there are still unresolved blockers.

@meisterT
Copy link
Member

I think this is gone by now, isn't it? cc @c-parsons

@jmmv jmmv added team-Rules-Server Issues for serverside rules included with Bazel untriaged and removed P1 I'll work on this now. (Assignee required) z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple labels May 13, 2020
@lberki lberki added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Nov 18, 2020
@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 16, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@sgowroji sgowroji closed this as not planned Won't fix, can't repro, duplicate, stale Feb 16, 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-Server Issues for serverside rules included with Bazel type: process
Projects
None yet
Development

No branches or pull requests

10 participants