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

port buildgen to py3 #6110

Merged
merged 3 commits into from Jul 15, 2018

Conversation

Projects
None yet
5 participants
@GoingTharn
Copy link
Contributor

GoingTharn commented Jul 13, 2018

Problem

Porting buildgen to py3. Needs references to str and object.

Solution

added builtins import for str and object

@GoingTharn

This comment has been minimized.

Copy link
Contributor

GoingTharn commented Jul 13, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 13, 2018

Hey, this will need a rebase to master. Sorry for the trouble!

@stuhood stuhood merged commit da2dd07 into pantsbuild:master Jul 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mateor

This comment has been minimized.

Copy link
Member

mateor commented Jul 15, 2018

thanks John! Feel free to add me to the reviewer lists. I am in the middle of moving but I will continue to swing by and look at patches once a day or so.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 15, 2018

The BUILD file is missing the dependency for “3rdparty/python:future”. Stu, you may want to revert this commit if or manually add that line before doing the release.

I’m surprised the tests are passing on this and that we don’t get an import error.

This slipped through because our script was only smart enough to add the line in certain cases. We updated our script so that it will always add the line when necessary, so going forward this shouldn’t happen.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 15, 2018

@Eric-Arellano ths is why tests pass:

$ ./pants depmap --tree --minimal contrib/buildgen/tests/python/pants_test/contrib/buildgen:build_file_manipulator | head
--internal-contrib.buildgen.tests.python.pants_test.contrib.buildgen.build_file_manipulator
  |--internal-contrib.buildgen.src.python.pants.contrib.buildgen.build_file_manipulator
  |  |--internal-src.python.pants.build_graph.build_graph
  |  |  |--internal-3rdparty.python.six
  |  |  |--internal-3rdparty.python.twitter.commons.twitter.common.collections
  |  |  |--internal-src.python.pants.base.build_environment
  |  |  |  |--internal-3rdparty.python.future
  |  |  |  |--internal-src.python.pants.version
  |  |  |  |  |--internal-3rdparty.python.packaging
  |  |  |  |  |--internal-src.python.pants.version-resource
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 15, 2018

Ah that makes more sense, thanks John.

We still probably to explicitly add to the BUILD file in case this assumption breaks for whatever reason, right?

We can submit a separate PR patching this if we don’t want to revert this merge.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 15, 2018

Yes, definitely. This is a bug to be fixed. No need to revert though and a new PR would be perfect.

stuhood added a commit that referenced this pull request Jul 16, 2018

Add missing future dependency to BUILD (#6135)
Fixing issue from the port of buildgen of missing dependency in BUILD file from #6110.

While technically everything runs correctly without this declaration, that is an implementation detail that should not be relied upon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment