Skip to content

protobuf: apply patch for python 3.7 only when building with it#51808

Closed
samueldr wants to merge 1 commit intoNixOS:masterfrom
samueldr:fix/protobuf-python37-patch
Closed

protobuf: apply patch for python 3.7 only when building with it#51808
samueldr wants to merge 1 commit intoNixOS:masterfrom
samueldr:fix/protobuf-python37-patch

Conversation

@samueldr
Copy link
Member

Change introduced in #50672.

The patch does not apply on older protobuf like protobuf3_1

$ nix-build -E 'with import ./. { }; python3Packages.protobuf.override { protobuf = protobuf3_1; }'
unpacking sources
unpacking source archive /nix/store/1zdyl0cxaa8ha2v1zp75zzdjd6j99d0m-source
source root is source
setting SOURCE_DATE_EPOCH to timestamp 315619200 of file source/util/python/BUILD
patching sources
applying patch /nix/store/yagx7hvylnnjq7lxbcia0y5lq1r736w3-0a59054c30e4f0ba10f10acfc1d7f3814c63e1a7.patch
patching file google/protobuf/pyext/descriptor.cc
Hunk #1 succeeded at 55 (offset -1 lines).
patching file google/protobuf/pyext/descriptor_containers.cc
patching file google/protobuf/pyext/descriptor_pool.cc
Hunk #1 succeeded at 47 (offset -1 lines).
patching file google/protobuf/pyext/extension_dict.cc
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file google/protobuf/pyext/extension_dict.cc.rej
patching file google/protobuf/pyext/message.cc
Hunk #1 succeeded at 82 (offset 3 lines).
Hunk #2 succeeded at 1425 (offset -104 lines).

Since the patch isn't necessary on python versions older than 3.7, let's
only apply it for version 3.7. This means that most things using older
protobuf implementation will now be able to build when using an older
pythonPackage set (as is most probably the case anyway).

This still leaves protobuf 3.1 using packages hanging, but the errors
will be localized to those that would be breaking anyway with the
upgrade to 3.7 as default python.

This can be tested using:

nix-build -E 'with import ./. { }; python36Packages.protobuf.override { protobuf = protobuf3_1; }'
Things done
  • ✔️ Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • ✔️ NixOS
    • ⬜ macOS
    • ⬜ other Linux distributions
  • ⬜ Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • ⬜ Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • ⬜ Tested execution of all binary files (usually in ./result/bin/)
  • ⬜ Determined the impact on package closure size (by running nix path-info -S before and after)
  • ✔️ Assured whether relevant documentation is up to date
  • ✔️ Fits CONTRIBUTING.md.

cc @lopsided98 @worldofpeace

@samueldr samueldr requested a review from FRidh as a code owner December 10, 2018 03:26
@samueldr samueldr force-pushed the fix/protobuf-python37-patch branch 2 times, most recently from 982cdff to 475d64f Compare December 10, 2018 03:28
@GrahamcOfBorg GrahamcOfBorg added the 6.topic: python Python is a high-level, general-purpose programming language. label Dec 10, 2018
@samueldr
Copy link
Member Author

See #51809 for a package this fixes. Since the patch won't apply with python 3.6, changing to python 3.6 is not enough to fix caffe2.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Dec 10, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't pythonAtLeast make more sense here?

Copy link
Member

Choose a reason for hiding this comment

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

isPy37

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! the things you can learn about the python ecosystem ;).

@samueldr samueldr force-pushed the fix/protobuf-python37-patch branch 2 times, most recently from b24992a to 53decb6 Compare December 10, 2018 19:47
Change introduced in NixOS#50672.

The patch does not apply on older protobuf like protobuf3_1

```
$ nix-build -E 'with import ./. { }; python3Packages.protobuf.override { protobuf = protobuf3_1; }'
unpacking sources
unpacking source archive /nix/store/1zdyl0cxaa8ha2v1zp75zzdjd6j99d0m-source
source root is source
setting SOURCE_DATE_EPOCH to timestamp 315619200 of file source/util/python/BUILD
patching sources
applying patch /nix/store/yagx7hvylnnjq7lxbcia0y5lq1r736w3-0a59054c30e4f0ba10f10acfc1d7f3814c63e1a7.patch
patching file google/protobuf/pyext/descriptor.cc
Hunk #1 succeeded at 55 (offset -1 lines).
patching file google/protobuf/pyext/descriptor_containers.cc
patching file google/protobuf/pyext/descriptor_pool.cc
Hunk #1 succeeded at 47 (offset -1 lines).
patching file google/protobuf/pyext/extension_dict.cc
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file google/protobuf/pyext/extension_dict.cc.rej
patching file google/protobuf/pyext/message.cc
Hunk #1 succeeded at 82 (offset 3 lines).
Hunk #2 succeeded at 1425 (offset -104 lines).
```

Since the patch isn't necessary on python versions older than 3.7, let's
only apply it for version 3.7. This means that most things using older
protobuf implementation will now be able to build when using an older
pythonPackage set (as is most probably the case anyway).

This still leaves protobuf 3.1 using packages hanging, but the errors
will be localized to those that would be breaking anyway with the
upgrade to 3.7 as default python.
@samueldr samueldr force-pushed the fix/protobuf-python37-patch branch from 53decb6 to 2ae8dd3 Compare December 10, 2018 19:48
@samueldr
Copy link
Member Author

Updated with isPy37.

@dotlambda
Copy link
Member

I'll merge #51809 once nix-review is done, hence closing this one.

@dotlambda dotlambda closed this Dec 11, 2018
@samueldr samueldr deleted the fix/protobuf-python37-patch branch February 12, 2019 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants