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

Bump python-gflags to 3.1.0, take two. #3363

Closed
wants to merge 2 commits into from

Conversation

spxtr
Copy link
Contributor

@spxtr spxtr commented Jul 11, 2017

c/8250 caused a breakage on windows. I don't have a windows machine to test this on, so I'll have to lean on Jenkins. Also, changing to GitHub because I am way more familiar with this interface than with Gerrit.

@spxtr
Copy link
Contributor Author

spxtr commented Jul 11, 2017

@damienmg PTAL. I still see lots of failures from android tests, but I'm not sure that those are related to this PR. The windows test that failed to build before is now passing, I think. It's hard to tell what with all the java/android failures.

@damienmg
Copy link
Contributor

Ok the failure seems genuine, added the android support experts to look at it and advise.

@aj-michael
Copy link
Contributor

The problem is in the jenkins log:

no such package '@bazel_tools//third_party/py/six': BUILD file not found on package path and referenced by '@bazel_tools//third_party/py/gflags:gflags'

We bundle gflags with bazel into the @bazel_tools repository, but we do not currently do that for six. This CL adds a dep from gflags to six, so we need to bundle six as well. You need to update //src:embedded_tools to have a dep on //third_party/py/six:srcs.

@damienmg
Copy link
Contributor

Humm this has to be done as a separate PR because 3rd party and non 3rd party code are like 2 repository for us.

bazel-io pushed a commit that referenced this pull request Jul 19, 2017
#3363 (comment)

Closes #3369.

Change-Id: I52c7c39db13131bfc343666fbd05840815ee7fa8
PiperOrigin-RevId: 162478002
@spxtr
Copy link
Contributor Author

spxtr commented Jul 19, 2017

retest this please

Change-Id: I3cbbd9d411520697f5457d9d6bc06e0d0e300fa3
@damienmg
Copy link
Contributor

Test failure does looks unrelated, good to merge tomorrow, hopefully we do not encounter any more hurdle inside Google.

@spxtr
Copy link
Contributor Author

spxtr commented Jul 24, 2017

Thanks :)

@spxtr
Copy link
Contributor Author

spxtr commented Jul 27, 2017

@damienmg any update?

@damienmg
Copy link
Contributor

damienmg commented Jul 27, 2017 via email

@spxtr
Copy link
Contributor Author

spxtr commented Jul 27, 2017

No worries.

@damienmg damienmg assigned aj-michael and unassigned damienmg Jul 28, 2017
@damienmg
Copy link
Contributor

Sorry I haven't got time to deal with it, trying to finish things before going on holidays. Reassigning to @aj-michael. I know they are some concern about version mistchmatch between the version of gflags in google and gflags 3, but so far it is not a problem.

@dslomov
Copy link
Contributor

dslomov commented Aug 2, 2017

retest this please

@dslomov
Copy link
Contributor

dslomov commented Aug 2, 2017

CI looks good

@aj-michael
Copy link
Contributor

Sorry for the delay, I just realized this was assigned to me and I do not have much time this week.

This change looks fine for Bazel, however, internally we still use the older gflags and I suspect it will not be easy to migrate. I am not sure what the teams thoughts are on trying to keep Bazel compatible with both versions, especially since I suspect the reason to update it in Bazel is to use new features, which will then break internal Bazel.

@spxtr
Copy link
Contributor Author

spxtr commented Aug 2, 2017

The reason for using it in Bazel is actually just that the newer version supports python 3. It's not really for any new features.

@dslomov
Copy link
Contributor

dslomov commented Aug 4, 2017

I'll merge this

@dslomov dslomov self-assigned this Aug 4, 2017
@bazel-io bazel-io closed this in d7f5c12 Aug 10, 2017
@spxtr spxtr deleted the gflagstaketwo branch August 14, 2017 20:41
@srkukarni srkukarni mentioned this pull request Aug 17, 2017
buchgr pushed a commit that referenced this pull request Aug 21, 2017
This reverts commit 1fb46ce.
Trying again.

Closes #3363
buchgr pushed a commit that referenced this pull request Aug 22, 2017
This reverts commit 1fb46ce.
Trying again.

Closes #3363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants