Skip to content

Remove dead objc setup code#12073

Closed
keith wants to merge 9 commits intobazelbuild:masterfrom
keith:ks/remove-dead-objc-setup-code
Closed

Remove dead objc setup code#12073
keith wants to merge 9 commits intobazelbuild:masterfrom
keith:ks/remove-dead-objc-setup-code

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Sep 9, 2020

This appears to be unused

@keith keith requested a review from lberki as a code owner September 9, 2020 23:15
@keith keith mentioned this pull request Sep 9, 2020
Closed
@keith
Copy link
Copy Markdown
Member Author

keith commented Sep 9, 2020

@trybka continuing from #12046 (comment)

if no one references LIBTOOL_ATTRIBUTE does that mean this is unreferenced entirely? I assumed that the registration of the LibtoolRule might mean it could be used implicitly somehow?

@trybka
Copy link
Copy Markdown
Contributor

trybka commented Sep 9, 2020

Good find on LibtoolRule. My read of this is that any caller which would want to invoke this libtool would need to grab it via the attribute (or calling the getter), and since I can't find those references, I think the whole mess is unused? Curious how far on the thread this can be pulled out.

@keith
Copy link
Copy Markdown
Member Author

keith commented Sep 9, 2020

I will push a second commit with that assumption after I validate I can build our app without that attribute

@keith
Copy link
Copy Markdown
Member Author

keith commented Sep 9, 2020 •
edited
Loading

if all it was doing was adding that implicit attribute, I would assume that someone would have to reference $libtool (even if not with the constant) to use it? similar to

bazel/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

Lines 245 to 246 in 24a082d

} else if (ruleContext.isAttrDefined("$lcov_merger", LABEL)) {
lcovMergerAttr = "$lcov_merger";

or

bazel/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java

Line 1452 in 24a082d

Artifact pruner = ruleContext.getPrerequisiteArtifact("$j2objc_dead_code_pruner");

@trybka
Copy link
Copy Markdown
Contributor

trybka commented Sep 9, 2020

if all it was doing was adding that implicit attribute, I would assume that someone would have to reference $libtool (even if not with the constant) to use it?

That's my understanding, yes.

@trybka
Copy link
Copy Markdown
Contributor

trybka commented Sep 9, 2020

+cc @googlewalt as an FYI

keith
keith commented Sep 10, 2020
View reviewed changes
Comment thread tools/objc/gcov_stub
@@ -1,7 +0,0 @@
#!/bin/bash
Copy link
Copy Markdown
Member Author

@keith keith Sep 10, 2020

Choose a reason for hiding this comment

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

hrm based on the code in rules_apple, this might work inside google? https://github.com/bazelbuild/rules_apple/blob/0a1cbce9b3d169269313aba1113d2aa0a8c3c514/apple/internal/testing/apple_test_rule_support.bzl#L124-L128

trybka
trybka reviewed Sep 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@trybka trybka left a comment

Choose a reason for hiding this comment

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

Let's put the apple_sdk flag back for now, there are more references to it internally, and we'd need to clean them up.

You can probably still remove the getters and whatnot

Comment thread src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java
@keith
Copy link
Copy Markdown
Member Author

keith commented Sep 10, 2020

added that flag back

@bazel-io bazel-io closed this in f8b51ff Sep 11, 2020
@keith keith deleted the ks/remove-dead-objc-setup-code branch September 11, 2020 15:30
@keith
Copy link
Copy Markdown
Member Author

keith commented Sep 11, 2020

Thanks a ton!~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trybka trybka trybka left review comments

@lberki lberki Awaiting requested review from lberki lberki is a code owner

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@keith @trybka @googlebot