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
python2Packages.google_resumable_media: fix tests #97768
Conversation
@GrahamcOfBorg build python2Packages.google_resumable_media |
@ofborg build python37Packages.google_resumable_media |
The
|
Ok I'll add google-crc32c |
9e21f36
to
96bec94
Compare
Had to add new package and update google_resumable_media to have the tests working |
export LDFLAGS="-L${pkgs.crc32c}/lib" | ||
export CFLAGS="-I${pkgs.crc32c}/include" | ||
''; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we're probably overwhelming you here but if you do
src = fetchFromGitHub {
owner = "googleapis";
repo = "python-crc32c";
rev = "v${version}";
sha256 = "0n3ggsxmk1fhq0kz6p5rcj4gypfb05i26fcn7lsawakgl7fzxqyl";
};
...
checkInputs = [ pytestCheckHook ];
pythonImportsCheck = [ "google_crc32c" ];
we should get the tests working. We should at least have the pythonImportsCheck
otherwise we have zero warning that this package is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no probem always love to add more tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum it broke the tests, apparently the c library is no more found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like it is the LDFLAGS/CFLAGS changes you proposed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok it is the pytestCheckHook that fails, it cannot load the module when this hook is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh .. I tried to force c++11 and it told me it is C, not C++, that must be the problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup we're finding this in parallel re the shared lib - it works if we set cmakeFlags = [ "-DBUILD_SHARED_LIBS=1" ];
in pkgs.crc32c
. The annoying thing is it won't build both the static and shared libs at the same time, and I'm not sure we want to take away the static lib from packages that might be happily using it. Just checking how many packages it actually affects...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we do is add a shared
argument to pkgs/development/libraries/crc32c/default.nix
, defaulting to false
, which just does a cmakeFlags = [ "-DBUILD_SHARED_LIBS=${if shared then "1" else "0"}" ];
. Then we just pass crc32c.override { shared = true; }
to this package in python-packages.nix
.
Thoughts @jonringer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually you opt into static.
I would add a staticOnly ? false
input.
Then in pkgs/top-level/static.nix
you would add:
crc = super.crc.override {
staticOnly = true;
};
and adjust the build flags accordingly
that way, when you do pkgsStatic.crc32
you get the .a
, but normally you get the .so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied these fixes and built locally successfully 🎉 :
- pkgs.crc32c
- pkgsStatic.crc32c
- pkgs.python2Packages.google_resumable_media
- pkgs.python37Packages.google_resumable_media
- pkgs.python38Packages.google_resumable_media
4cff45f
to
c0bb59c
Compare
c0bb59c
to
9375ffa
Compare
the crc32 changes should be their own commit
otherwise LGTM |
9375ffa
to
e12c10d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
failures are broken on target branch
https://github.com/NixOS/nixpkgs/pull/97768
3 packages failed to build:
google-cloud-cpp python37Packages.google_cloud_bigquery python38Packages.google_cloud_bigquery
11 packages built:
crc32c dvc-with-remotes python27Packages.google_resumable_media python37Packages.google-crc32c python37Packages.google_cloud_automl python37Packages.google_cloud_storage python37Packages.google_resumable_media python38Packages.google-crc32c python38Packages.google_cloud_automl python38Packages.google_cloud_storage python38Packages.google_resumable_media
Motivation for this change
Adding crcmod for tests
ZHF: #97479
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)