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

Make AndroidTests pass with incompatible_disallow_empty_glob #15374

Closed
wants to merge 1 commit into from
Closed

Make AndroidTests pass with incompatible_disallow_empty_glob #15374

wants to merge 1 commit into from

Conversation

limdor
Copy link
Contributor

@limdor limdor commented Apr 29, 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.

@ahumesky
Copy link
Contributor

So were each of these globs actually failing with --incompatible_disallow_empty_glob? Checking a few of these for example, the globs in android_ndk_misc_libraries_template.txt, the cpufeatures files are still in the NDK, so those globs should match something

@limdor
Copy link
Contributor Author

limdor commented Apr 29, 2022

How I came up with these globs and not others?
These allow_empty True were needed to flip the flag #15327
The same for #15339 and #15330
I splitted in 3 to make it easier but I can also put them together or split them per folder.
Just let me know what would be the preference.

Once these 3 are merged, the flag still cannot be flipped because of protocolbuffers/upb#584

@ahumesky
Copy link
Contributor

So adding allow_empty = True would move us closer to enabling the flag, but it would seem to be better to investigate why each of these globs is empty. And again in the example of cpufeatures, the NDK still has for example sources/android/cpufeatures/cpu-features.c so that glob shouldn't be returning nothing -- there might actually be something wrong with the glob or something else.

Is there someone on the bazel team who you're working with to drive this effort?

At minimum it would be good to create a separate issue to attach as a TODO to each glob that has allow_empty = True added to it so that we know why the code is the way that it is.

@limdor
Copy link
Contributor Author

limdor commented Apr 29, 2022

Is there someone on the bazel team who you're working with to drive this effort?

No, who would be the right person?

And again in the example of cpufeatures, the NDK still has for example sources/android/cpufeatures/cpu-features.c so that glob shouldn't be returning nothing -- there might actually be something wrong with the glob or something else.

The problem is not with the example but when running the test //src/test/java/com/google/devtools/build/lib/bazel/rules/android:AndroidTests

At minimum it would be good to create a separate issue to attach as a TODO to each glob that has allow_empty = True added to it so that we know why the code is the way that it is.

If you prefer I can do that

@limdor
Make AndroidTests pass with incompatible_disallow_empty_glob
1398526 
One of the reasons why is not possible to flip
incompatible_disallow_empty_glob is because AndroidTests are failing

//src/test/java/com/google/devtools/build/lib/bazel/rules/android:AndroidTests

This commit add allow_empty = True to reflect the default behavior and
make the tests pass.
@limdor limdor changed the title Be explicit about alowing empty globs (third part) Make AndroidTests pass with incompatible_disallow_empty_glob Apr 29, 2022
@limdor
Copy link
Contributor Author

limdor commented Apr 29, 2022

@ahumesky I've updated the changes to include only the changes strictly needed for AndroidTests. If you think that this is not ok then probably the tests should be adapted instead.

@ckolli5 ckolli5 added the team-Android Issues for Android team label Apr 29, 2022
limdor
limdor commented Apr 30, 2022
View reviewed changes
src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -186,7 +186,7 @@ public void setupMockClient(MockToolsConfig config, List<String> workspaceConten
"java_host_runtime_alias(name = 'current_host_java_runtime')",
"filegroup(name='langtools', srcs=['jdk/lib/tools.jar'])",
"filegroup(name='bootclasspath', srcs=['jdk/jre/lib/rt.jar'])",
"filegroup(name='extdir', srcs=glob(['jdk/jre/lib/ext/*']))",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one comment about this. It does not look like android specific but it is needed also to make the android tests work. Maybe someone else apart from the android team should look at it.

@limdor
Copy link
Contributor Author

limdor commented Jul 23, 2022

@ahumesky @ted-xie friendly reminder.
Could you give me an update on what would be needed in your opinion in order to move forward? Thanks

@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 24, 2022
@limdor limdor mentioned this pull request Aug 24, 2022
Open
56 tasks
@limdor
Copy link
Contributor Author

limdor commented Sep 27, 2022

Gentle reminder

@aiuto aiuto assigned sgowroji and ted-xie and unassigned sgowroji Sep 29, 2022
@aiuto aiuto self-requested a review September 29, 2022 04:01
aiuto
aiuto reviewed Sep 29, 2022
View reviewed changes
...va/com/google/devtools/build/lib/bazel/rules/android/android_ndk_misc_libraries_template.txt
@@ -1,6 +1,6 @@
cc_library(
name = "cpufeatures",
srcs = glob(["ndk/sources/android/cpufeatures/*.c"]),
hdrs = glob(["ndk/sources/android/cpufeatures/*.h"]),
srcs = glob(["ndk/sources/android/cpufeatures/*.c"], allow_empty = True),
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't make sense to me that these could ever result in failed globs in real life, but maybe they do in our tests.
OTOH, I don't think it is a problem that we hide the glob failure. We will simply push the error from glob time until link time, but the build will still fail, so all is good.

The same question applies everywhere.

aiuto
aiuto reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@aiuto aiuto left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm no nothing about these rules.

@meteorcloudy meteorcloudy mentioned this pull request Sep 29, 2022
Open
@ted-xie
Copy link
Contributor

ted-xie commented Sep 29, 2022

It also seems strange to me that any of these globs would return empty. I would expect downstream failures if various NDK features are missing source files. I'm OK with pushing this in upstream, but we should all be aware that something else entirely may be broken that's causing the source files not to show up.

aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
@limdor @aiuto
Make AndroidTests pass with incompatible_disallow_empty_glob
9480be1 
There are several globs that are empty and this prevents building
with the incompatible flag bazelbuild#8195.
This commit just makes it explicit that empty is allowed.

Closes bazelbuild#15374.

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

@aiuto aiuto aiuto left review comments

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

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

Assignees

@ted-xie ted-xie

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.
6 participants
@limdor @ahumesky @ted-xie @aiuto @sgowroji @ckolli5