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

Jump to bottom

pythonPackages.libarcus: use pkgs.protobuf as input #56696

Merged
merged 2 commits into from Mar 5, 2019
Merged

pythonPackages.libarcus: use pkgs.protobuf as input #56696

merged 2 commits into from Mar 5, 2019

Conversation

jokogr
Copy link
Contributor

@jokogr jokogr commented Mar 2, 2019

Motivation for this change

Cura was not building because of libarcus.

Since 5c75318 the protobuf Python module does not have protobuf library as a propagated build input.

This commit makes pkgs.protobuf the input for libarcus, since the latter
searches for the native protobuf library and not for the Python module.

I have tested this by building and running Cura.

/cc @FRidh

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.

@jokogr jokogr requested a review from FRidh as a code owner March 2, 2019 13:18
@dotlambda dotlambda changed the title libarcus: use pkgs.protobuf as input pythonPackages.libarcus: use pkgs.protobuf as input Mar 2, 2019
@dotlambda
Copy link
Member

dotlambda commented Mar 2, 2019 •
edited

@GrahamcOfBorg build python3.pkgs.libarcus

dotlambda
dotlambda requested changes Mar 2, 2019
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Please move protobuf from propagatedBuildInputs to buildInputs.

@dotlambda dotlambda added the 9.needs: port to stable A PR needs a backport to the stable release. label Mar 2, 2019
@dotlambda
Copy link
Member

This should be backported to staging-19.03.

@jokogr
Copy link
Contributor Author

jokogr commented Mar 2, 2019

@dotlambda If I move protobuf to buildInputs, curaengine still fails as in:

-- Building with Arcus
CMake Error at /nix/store/a4icwnjm9iasay87ggkfp4gw36wsqcpw-cmake-3.13.4/share/cmake-3.13/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find Protobuf (missing: Protobuf_INCLUDE_DIR) (Required is at
  least version "3.0.0")
Call Stack (most recent call first):
  /nix/store/a4icwnjm9iasay87ggkfp4gw36wsqcpw-cmake-3.13.4/share/cmake-3.13/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /nix/store/a4icwnjm9iasay87ggkfp4gw36wsqcpw-cmake-3.13.4/share/cmake-3.13/Modules/FindProtobuf.cmake:595 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  /nix/store/2dcds8m1xi8x11qxzk4czjcxq6xj6xlc-python3.7-libarcus-3.6.0/lib/cmake/Arcus/ArcusConfig.cmake:31 (find_package)
  CMakeLists.txt:14 (find_package)

Adding protobuf in curaengine's buildInputs explicitly while protobuf is in libarcus' buildInputs also works, are you suggesting doing so?

I think it's more elegant to add just libarcus on packages and the latter to use libarcus CMake library to get protobuf, but we are talking about 2 packages currently relying on libarcus (curaengine and uranium, which is actually used to build cura).

@dotlambda
Copy link
Member

Adding protobuf in curaengine's buildInputs explicitly while protobuf is in libarcus' buildInputs also works, are you suggesting doing so?

Yes, definitely.

@jokogr jokogr requested a review from dotlambda March 2, 2019 15:05
@delroth
Copy link
Contributor

delroth commented Mar 3, 2019

I think we should instead add back pkgs.protobuf to propagatedNativeBuildInputs for pythonPackages.protobuf. This recent change has broken more than libarcus -- python3Packages.trezor is also broken.

delroth referenced this pull request Mar 3, 2019
@FRidh
python.pkgs.protobuf: fix build
5c75318
@delroth
Copy link
Contributor

delroth commented Mar 3, 2019

Actually this failure is not fixed with pkgs.protobuf in propagatedNativeBuildInputs -- different root cause from the python3Packages.trezor problem. I'll file a separate PR.

@jokogr
Copy link
Contributor Author

jokogr commented Mar 5, 2019

@delroth as you said, 5c75318 did not fix the problem with libarcus.

@dotlambda I have made the requested changes, could you review and tell me if something more is needed?

@jokogr
pythonPackages.libarcus: use pkgs.protobuf as input
6a523a1 
This commit makes pkgs.protobuf the input for libarcus, since the latter
searches for the native protobuf library and not for the Python module.
@jokogr
curaengine: add protobuf in buildInputs
c14eed2
@dotlambda
Copy link
Member

@GrahamcOfBorg build python2.pkgs.libarcus python3.pkgs.libarcus curaengine

@dotlambda dotlambda merged commit 9b3e5a3 into NixOS:master Mar 5, 2019
@jokogr jokogr deleted the f/libarcus branch March 5, 2019 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh Awaiting requested review from FRidh

@dotlambda dotlambda Awaiting requested review from dotlambda

@abbradar abbradar Awaiting requested review from abbradar

Assignees
No one assigned
Labels
6.topic: python 9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jokogr @dotlambda @delroth @GrahamcOfBorg