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
mapbox-gl-native: use upstream version plus patch #117052
mapbox-gl-native: use upstream version plus patch #117052
Conversation
This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 3 packages built:
|
Result of 3 packages built successfully:2 suggestions:
|
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.
This looks good to me!
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.
Looks good, but I think the patch name should match the commit it's from and we might as well add the other patch now to save me adding it later 😝
fetchSubmodules = true; | ||
}; | ||
|
||
patches = [ | ||
(fetchpatch { | ||
name = "add-support-for-using-qmapboxgl-as-proper-cmake-dependency.patch"; |
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.
This commit is actually called "Add support for qmapboxgl installation"
I think we should also add "add-support-for-using-qmapboxgl-as-proper-cmake-dependency" from the same PR as the first patch too: with it, I believe we'll be able to update to the latest mapbox-gl-qml version, switch to building mapbox-gl-qml with cmake and drop some hacks.
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.
Oops, I took the wrong commit. I'll fix it later.
Can someone from @NixOS/qt-kde confirm whether it was correct to move these libraries to |
1f0e912
to
84e3856
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.
I think the move to qt5-packages.nix
is good, and that the PR can consist of two squashed commits - 1 for each package.
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.
Agree with squashing, but otherwise looks really good to me.
…to libsForQt5 The first two patches are needed for mapbox-gl-qml. The third patch is required when building without the vendored rapidjson.
84e3856
to
cccebb6
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.
Looks good to me! Thanks!
Motivation for this change
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)