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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
proj-data: init at 1.16.0 #280062
proj-data: init at 1.16.0 #280062
Conversation
ff29980
to
971bee9
Compare
d6f0028
to
e8200c0
Compare
@@ -58,6 +62,10 @@ stdenv.mkDerivation (finalAttrs: rec { | |||
|
|||
doCheck = true; | |||
|
|||
postInstall = lib.optionalString withProjData '' | |||
cp -R ${proj-data}/* $out/share/proj/ |
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.
cp -R ${proj-data}/* $out/share/proj/ | |
cp --recursive "${proj-data}" "$out/share/proj" |
- GNU
cp
supports all of-R, -r, --recursive
, and the last one is the most obvious. - If I understand correctly
$out/share/proj
doesn't exist at this point, so we can copy the entire directory across.*
doesn't match dotfiles by default, so this also ensures that we copy everything. - Quote variables for general safety.
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.
- GNU cp supports all of -R, -r, --recursive, and the last one is the most obvious.
Done. Thanks
- If I understand correctly $out/share/proj doesn't exist at this point
Yes, it already exists and contains some files. No grid files should be hidden files, so I have no worries about not copying something what should be.
- Quote variables for general safety.
I understand this point, but most of path variables in nixpkgs code (including the ones in this package) are not quoted. Nix store path can't contain any strange characters or spaces. I am leaving this unquoted for consistency.
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.
(Feel free to close any discussions where I've 馃憤'ed your last reply - I don't have access to close discussions.)
runHook preInstall | ||
shopt -s extglob | ||
|
||
mkdir -p $out |
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.
mkdir -p $out | |
mkdir -p "$out" |
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.
See the comment about store path variables quoting above.
shopt -s extglob | ||
|
||
mkdir -p $out | ||
cp README.DATA $out/README-PROJ-DATA.md |
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.
cp README.DATA $out/README-PROJ-DATA.md | |
cp README.DATA "$out"/README-PROJ-DATA.md |
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.
See the comment about store path variables quoting above.
cp README.DATA $out/README-PROJ-DATA.md | ||
|
||
for grid in ${builtins.toString gridPackages}; do | ||
if [ ! -d $grid ]; then |
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.
Isn't this check redundant? The cp
will fail if the directory doesn't exist.
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.
If this check doesn't exist, the error message is quite confusing
> Running phase: installPhase
> cp: missing destination file operand after '/nix/store/v3258iwkrrmh5iddli7sdzxgikiw3cyh-proj-data-1.16.0/'
> Try 'cp --help' for more information.
```.
I prefer to leave it as is.
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.
Sounds good. In that case I'd recommend changing the check to if ! [[ -d "$grid" ]]
- it won't produce a syntax error if anyone includes an empty grid package by accident.
|
||
for grid in ${builtins.toString gridPackages}; do | ||
if [ ! -d $grid ]; then | ||
echo "ERROR: Grid ($grid) does not exist." |
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 should probably go to stderr:
echo "ERROR: Grid ($grid) does not exist." | |
echo "ERROR: Grid ($grid) does not exist." >&2 |
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.
Thanks, good point. Done
exit 1 | ||
fi | ||
|
||
cp $grid/!(*.sh|*.py) $out/ |
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.
cp $grid/!(*.sh|*.py) $out/ | |
cp "$grid"/!(*.sh|*.py) "$out"/ |
That said, would it be possible to create a positive rather than negative glob to include the relevant files? Something like *.json *.tif *.tiff *.txt
?
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.
Good point, but it is very hard to create future-proof list of allowed file types. I would prefer to leave it as is for now.
runCommand "${pname}-tests" { meta.timeout = 60; } | ||
'' | ||
# add proj-data files to proj resources search path | ||
export PROJ_DATA=${proj}/share/proj:${proj-data} |
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.
Should this possibly be set by the package itself?
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 am not sure what is the best approach. We can wrap all executables with PROJ_DATA
path, but it wouldn't help with dozens of other apps using proj
as library (like QGIS for example).
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 am not sure what is the best approach. We can wrap all executables with
PROJ_DATA
path, [鈥
Sounds good.
but it wouldn't help with dozens of other apps using
proj
as library (like QGIS for example).
Isn't that unrelated? We'd be making one thing work without breaking anything else, right?
let | ||
# build custom package containing `nz_linz` grids | ||
projDataWithGrid = finalAttrs.finalPackage.overrideAttrs (_: { | ||
postInstall = "cp nz_linz/* $out/"; |
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.
Shouldn't this override gridPackages
instead?
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.
Yes, I would like to, but finalPackage
doesn't support override
(it supports only overrideAttrs
). Any suggestions are welcome.
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 don't know Nix well enough to suggest anything. Feel free to ignore this, unless someone else has a suggestion.
inherit (proj-data) pname; | ||
in | ||
runCommand "${pname}-tests" { meta.timeout = 60; } | ||
'' |
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 test is using pipelines, so set -o pipefail
might help.
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.
Good point, done.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/week-in-geospatial-team/37035/3 |
e8200c0
to
6904ffe
Compare
@dotlambda , are you OK with this PR ? |
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.
https://github.com/NixOS/nixpkgs/pull/280062/files#r1458004097 is still applicable.
Consider using |
I am replacing this PR with #290643 which is using better approach of adding grid packages to |
Description of changes
proj-data
package containing datum grid resource files forproj
package
proj-data
grid resource files inproj
data directoryAFAIK, installation of
proj-data
resource files inproj
directory is the only user-friendly way of enabling them for all programs usingproj
. Another alternative methods are:PROJ_DATA
variable to${proj}/share/proj:${proj-data}
(seeproj-data
package tests)which can be very tricky for other applications using
proj
in containers or multi-user environments and can be error-prone.For more information, see: https://proj.org/en/9.3/resource_files.html
Installation of
proj-data
files toproj
is optional, becauseproj-data
repository size is large (can reach 1GB of size soon) and it avoids downloading it when buildingproj
package version whereproj-data
is not needed.Example usage
proj-data
grids provided bynz_linz
:Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 馃憤 reaction to pull requests you find important.