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

Remove empty globs #16431

Closed
wants to merge 1 commit into from
Closed

Remove empty globs #16431

wants to merge 1 commit into from

Conversation

limdor
Copy link
Contributor

@limdor limdor commented Oct 8, 2022

There are several globs that are empty and this prevents building with the incompatible flag #8195.
This commit just makes it explicit that empty is allowed.

@limdor limdor mentioned this pull request Oct 8, 2022
Closed
56 tasks
@limdor limdor marked this pull request as ready for review October 8, 2022 17:28
@sgowroji sgowroji added team-Android Issues for Android team awaiting-review PR is awaiting review from an assigned reviewer labels Oct 9, 2022
@ahumesky
Copy link
Contributor

Thanks for cleaning this up. I think we can just remove all the attributes that are taking empty globs in the tests, they're probably just copy pasted from other tests.

Could you move the changes to tools/build_defs/pkg/BUILD and tools/jdk/jdk.BUILD to a separate PR, since that's not test code?

@limdor
Copy link
Contributor Author

limdor commented Oct 13, 2022

Thanks for cleaning this up. I think we can just remove all the attributes that are taking empty globs in the tests, they're probably just copy pasted from other tests.

Could you move the changes to tools/build_defs/pkg/BUILD and tools/jdk/jdk.BUILD to a separate PR, since that's not test code?

Sure, here you have #16468

@limdor limdor force-pushed the empty_globs branch 2 times, most recently from f96fecd to 201beaa Compare October 18, 2022 21:34
@limdor
Copy link
Contributor Author

limdor commented Oct 18, 2022

Thanks for cleaning this up. I think we can just remove all the attributes that are taking empty globs in the tests, they're probably just copy pasted from other tests.

Could you move the changes to tools/build_defs/pkg/BUILD and tools/jdk/jdk.BUILD to a separate PR, since that's not test code?

@ahumesky I removed the empty attributes as requested. Could you take a look? Thanks

@limdor
Remove empty globs
f55b354 
There are several globs that are empty and this prevents building
with the incompatible flag bazelbuild#8195.
This commit just makes it explicit by removing the attributes that
are just an empty list.
@limdor limdor changed the title Be explicit about alowing empty globs (third part) Remove empty globs Oct 18, 2022
@ahumesky
Copy link
Contributor

Great, thank you, I'll start importing this now

ahumesky
ahumesky reviewed Oct 19, 2022
View reviewed changes
src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -183,7 +183,6 @@ public void testFatApk_androidPlatformsFlag() throws Exception {
" proguard_specs = [],",
" deps = ['lib', 'native-lib'],",
" manifest = 'AndroidManifest.xml',",
" resource_files = glob(['res/**']))");
Copy link
Contributor

Choose a reason for hiding this comment

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

this has to be adjusted, but i can do that on the import

copybara-service bot pushed a commit that referenced this pull request Oct 7, 2024
@limdor @copybara-github
Flip disallow empty glob
538b119 
This PR is to flip the default value of `incompatible_disallow_empty_glob`. This flag is tracked in #8195 and it is available since Bazel 0.29 from August 2019.
Once this is merged, users still can allow empty globs without being explicit setting `--incompatible_disallow_empty_glob=false`.

The motivation of the flip is bazel-contrib/SIG-rules-authors#37 and the fact of not keeping an incompatible flag for 4 years without flipping it.

What needs to be done before flipping this:

- [x] Merge protocolbuffers/upb#584
- [x] Update upb to a version that has the change in protocolbuffers/upb#584
- [x] Merge protocolbuffers/upb#745
- [x] Bring the changes from protocolbuffers/upb#745 to the 21.x branch
- [x] Bring the changes from protocolbuffers/upb@e5f2601 to the 21.x branch (protocolbuffers/upb#781)
- [x] Update upb to the latest commit of the 21.x branch (#16343)
- [x] Merge #15374
- [x] Merge #15339
- [x] Merge #15330
- [x] Merge #16431
- [x] Merge #16468
- [x] Get green checks for this PR
- [ ] Fix failures in downstream projects: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags
  - [ ] Fix empty globs from IntellIJ Plugin (https://github.com/bazelbuild/intellij)
    - [ ] Issue: bazelbuild/intellij#4040
    - [ ] Android Studio Plugin
    - [ ] Android Studio Plugin Google
    - [ ] CLion Plugin
    - [ ] CLion Plugin Google
    - [ ] IntelliJ Plugin
    - [ ] IntelliJ Plugin Aspect
    - [ ] IntelliJ Plugin Aspect Google
    - [ ] IntelliJ Plugin Google
    - [ ] IntelliJ UE Plugin
    - [ ] IntelliJ UE Plugin Google
    - Opened PRs
      - [x] bazelbuild/intellij#4025
      - [x] bazelbuild/intellij#4038
  - [x] Fix empty globs from Bazel Bench (https://github.com/bazelbuild/bazel-bench)
    - [ ] bazelbuild/bazel-bench#174
    - [x] bazelbuild/bazel-bench#149
  - [x] Fix empty globs from Buildfarm (https://github.com/bazelbuild/bazel-buildfarm)
  - [x] Fix empty globs from Cargo
  - [x] ~~Fix empty globs from Cartographer (https://github.com/cartographer-project/cartographer)~~ (No longer maintained)
    - Issue: cartographer-project/cartographer#1908
    - PR to enforce it cartographer-project/cartographer#1944
  - [x] Fix empty globs from Cloud Robotics Core (https://github.com/googlecloudrobotics/core/)
    - [x] PR to prevent regresions googlecloudrobotics/core#279
  - [ ] Fix empty globs from Envoy
  - [x] Fix empty globs from FlatBuffers
  - [x] Fix empty globs from Flogger
  - [ ] Fix empty globs from Gerrit (https://gerrit.googlesource.com/gerrit.git)
    - Issue: https://issues.gerritcodereview.com/issues/40011754
  - [x] Fix empty globs from :bazel:Protobuf
  - [x] Fix empty globs from rules_android_ndk
  - [ ] Fix empty globs from rules_dotnet
  - [x] Fix empty globs from :bazel:rules_foreign_cc bazel-contrib/rules_foreign_cc#974
  - [x] Fix empty globs from rules_go
  - [x] Fix empty globs from rules_haskell tweag/rules_haskell#1827
  - [x] Fix empty globs from :bazel:rules_jvm_external
  - [x] Fix empty globs from rules_kotlin
  - [x] Fix empty globs from rules_nodejs
  - [x] Fix empty globs from :bazel:rules_python
  - [ ] Fix empty globs from rules_rust
  - [x] Fix empty globs from rules_swift
  - [x] Fix empty globs from rules_webtesting
  - [x] Fix empty globs from TensorFlow  tensorflow/tensorflow#58155
    - [x] tensorflow/tensorflow#58008
    - [x] tensorflow/tensorflow#58154
  - [x] Fix empty globs from upb

RELNOTES[INC]: The flag `--incompatible_disallow_empty_glob` now defaults to true.

Closes #15327.

PiperOrigin-RevId: 683316885
Change-Id: Id34ffd32a9e979fada9207e475b826a508100ba5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahumesky ahumesky ahumesky left review comments

@ted-xie ted-xie Awaiting requested review from ted-xie ted-xie is a code owner

Assignees
No one assigned
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Android Issues for Android team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@limdor @ahumesky @sgowroji