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

tests,windows: enable some of android.desugar.* #4797

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@laszlocsomor
Copy link
Contributor

laszlocsomor commented Mar 8, 2018

On Windows, enable some tests under
c.g.d.build.android.desugar by adding them to
the transitive closure of //src:all_windows_tests,
thus running them on CI.

Depends on PR #4796
See issue #4292

laszlocsomor added some commits Mar 8, 2018

tests,windows: enable android.desugar.runtime
Add the j.c.g.d.build.android.desugar.runtime
tests to the transitive closure of
//src:all_windows_tests, thus running them on CI.

See #4292
tests,windows: enable some of android.desugar.*
On Windows, enable some tests under
c.g.d.build.android.desugar by adding them to
the transitive closure of //src:all_windows_tests,
thus running them on CI.

See #4292

Change-Id: Ia2ff6e6b2bc83581d2d2ab2926d9ba40d44f96e2
@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Mar 8, 2018

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Mar 8, 2018

This PR requires either that we update the Windows config scripts on Buildkite to add the android_{s,n}dk_repository rules to the WORKSPACE, or that we uncomment those rules in the checked-in WORKSPACE file.

@philwo tells me that @ahumesky said that it should be safe to uncomment those rules even if you don't have the Android {S,N}DK installted. I haven't tried yet.

@aj-michael

This comment has been minimized.

Copy link
Contributor

aj-michael commented Mar 8, 2018

I do not think we should add android_sdk_repository to the main bazel WORKSPACE file. We have some selects that are effectively conditional on whether or not android_sdk_repository is run (see //external:has_androidsdk) that allow loading and analysis to succeed (but execution fail) on some tests that require an Android SDK.

https://source.bazel.build/search?q=%2F%2Fexternal:has_androidsdk&ss=bazel

Adding android_sdk_repository to WORKSPACE would break some users who are doing blaze query over //... if they don't have ANDROID_HOME set.

@lfpino

This comment has been minimized.

Copy link
Contributor

lfpino commented Mar 27, 2018

Hi all, Bazel sheriff here, what's the status of this pull request? It hasn't been updated for more than a week so it'd be good to understand if it's still ongoing or safe to close. Thanks.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Mar 28, 2018

I still want to merge this, but I need to fix the CI machine setup first.

@philwo

This comment has been minimized.

Copy link
Member

philwo commented Mar 28, 2018

I still want to merge this, but I need to fix the CI machine setup first.

What are the required changes on the CI machines?

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Mar 28, 2018

What are the required changes on the CI machines?

To patch the Windows WORKSPACE file to have the android_{s,n}dk_repository rules.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Mar 28, 2018

To patch the Windows WORKSPACE file to have the android_{s,n}dk_repository rules.

@philwo : Can you do that?

@philwo

This comment has been minimized.

Copy link
Member

philwo commented Mar 28, 2018

@laszlocsomor Will do, yes.

philwo added a commit that referenced this pull request Apr 16, 2018

@laszlocsomor laszlocsomor deleted the laszlocsomor:android-desugar-tests branch Jul 27, 2018

bazel-io pushed a commit that referenced this pull request Dec 14, 2018

Uncomment android_sdk/ndk_repository on Windows
Revive #5023 (by @philwo) so that we can try to revive #4797.

`shell_commands` will put the whole cmd string into an array (`[cmd]`) before passing it to `subprocess.run`. This will make `subprocess.run` thinks that the first element of the array is the full path of the program, hence the weird error in #5023. We work around it with `batch_commands` which pass the cmd string directly into `subprocess.run` and let `subprocess.run` do the heavy-lifting.

/cc @philwo @meteorcloudy

Closes #6699.

PiperOrigin-RevId: 225547132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment