Conversation
github-actions
bot
added
6.topic: python
github-actions
bot
added
10.rebuild-darwin: 1-10
There was a problem hiding this comment.
wouldn't it be better to install as just chasemapper without the .py?
There was a problem hiding this comment.
Probably, but it would conflict with the chasemapper directory
There was a problem hiding this comment.
not sure what you mean... is there a chasemapper dir in $out/bin?
There was a problem hiding this comment.
Yes there is - cp -r chasemapper $out/bin/
This folder is referenced in horusmapper.py along with the other folders that get copied
There was a problem hiding this comment.
I believe I addressed all feedback other than this
There was a problem hiding this comment.
I am wondering if it would be sensible to put everything into a non-bin folder and then just have a single line bash script at $out/bin/chasemapper that calls the python file in the non-bin folder. Would that make sense?
There was a problem hiding this comment.
Alright this is what I decided to do ^ figured putting everything in bin was hacky
|
@drupol ty! I believe I've addressed all your feedback |
scd31
force-pushed
the
add-chasemapper
branch
2 times, most recently
from
00a3fc3 to
b6136d0
Compare
scd31
force-pushed
the
add-chasemapper
branch
2 times, most recently
from
9c576e1 to
579b7f6
Compare
drupol
left a comment
drupol
left a comment
There was a problem hiding this comment.
Can you add tests for the chasemapper service? There are many examples in nixpkgs already, but feel free to ask for some help if needed.
|
@drupol added a test to confirm that it starts properly and listens on the HTTP port we expect (: |
| echo "#!${bash}/bin/bash" > $out/bin/chasemapper | ||
| echo $out/horusmapper.py '"$@"' >> $out/bin/chasemapper | ||
| chmod +x $out/bin/chasemapper |
There was a problem hiding this comment.
Wouldn't it be better to create that with writeShellApplication?
There was a problem hiding this comment.
Something like this?
installPhase = ''
runHook preInstall
mkdir -p $out/bin
cp -r chasemapper $out/
cp -r static $out/
cp -r templates $out/
install horusmapper.py $out/
cp (${
writeShellApplication {
name = "chasemapper";
text = "$out/horusmapper.py \"$@\"";
}
}) $out/bin/chasemapper
runHook postInstall
'';
Not really sure how to do this properly. It's not working because $out isn't bound in the inner string
| lib, | ||
| buildPythonPackage, | ||
| fetchFromGitHub, | ||
| deprecated, | ||
| hopcroftkarp, | ||
| joblib, | ||
| matplotlib, | ||
| numpy, | ||
| scikit-learn, | ||
| scipy, | ||
| pytestCheckHook, | ||
| pythonAtLeast, | ||
| pythonOlder, |
There was a problem hiding this comment.
Can you please clean all the parameters that are unused? Here and in the other files too.
| hash = "sha256-C8/5x8tim6s0hWgCC7LpN1hesdVME5kpQFqDTEyXHtg="; | ||
| }; | ||
|
|
||
| sourceRoot = "source/src"; |
There was a problem hiding this comment.
| sourceRoot = "source/src"; | |
| sourceRoot = "${finalAttrs.src.name}/src"; |
see #245388
| meta = { | ||
| description = "CUSF standalone predictor"; | ||
| homepage = "https://github.com/darksidelemm/cusf_predictor_wrapper"; | ||
| license = lib.licenses.gpl3; |
There was a problem hiding this comment.
| license = lib.licenses.gpl3; | |
| license = lib.licenses.gpl3Only; |
deprecated
Also, some files are licensed under the gpl2Plus and others are licensed under the mit.
There was a problem hiding this comment.
How do we want to resolve the different files having different licenses? Is it safe to use gpl3only since that's what the overarching repo is marked as?
| buildPythonPackage rec { | ||
| pname = "cusfpredict"; | ||
| version = "0.2.1"; | ||
| format = "setuptools"; |
There was a problem hiding this comment.
| format = "setuptools"; | |
| pyproject = true; |
deprecated
Please set build-system instead.
https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/python.section.md#buildpythonpackage-parameters-buildpythonpackage-parameters
There was a problem hiding this comment.
I must admit I'm having a bad time with this. I had to do a patch to get poetry to work:
postPatch = ''
substituteInPlace pyproject.toml \
--replace-fail poetry.masonry.api poetry.core.masonry.api \
--replace-fail "poetry>=" "poetry-core>="
'';
Now it's failing with the following:
> Checking runtime dependencies for cusfpredict-0.2.1-py3-none-any.whl
> - cfgrib not installed
> - fastkml not installed
> - numpy not installed
> - python-dateutil not installed
> - pytz not installed
> - requests not installed
> - shapely not installed
> - xarray not installed
Do I really need to add cfgrib (and possibly others) to nixpkgs to get back to the place we were in before?
| src = fetchFromGitHub { | ||
| owner = "darksidelemm"; | ||
| repo = "cusf_predictor_wrapper"; | ||
| rev = "f4352834a037e3e2bf01a3fd7d5a25aa482e27c6"; | ||
| hash = "sha256-C8/5x8tim6s0hWgCC7LpN1hesdVME5kpQFqDTEyXHtg="; | ||
| }; |
There was a problem hiding this comment.
Please share the source code with cusf-predictor-wrapper.
I think these should be combined into a single package.
There was a problem hiding this comment.
How would this work in practice? It's a monorepo but there seems to be two distinct projects in the repo, each with a separate build system. Can they be combined into one package? Furthermore, if you want one without the other, wouldn't you still need to build both if we combined them? Chasemapper itself only needs cusfpredict, not the wrapper
| thunderforest_api_key = lib.mkOption { | ||
| default = "none"; | ||
| description = '' | ||
| ThunderForest API Key | ||
| ''; | ||
| type = lib.types.str; | ||
| }; | ||
| stadia_api_key = lib.mkOption { | ||
| default = "none"; | ||
| description = '' | ||
| Stadia Maps API Key | ||
| ''; | ||
| type = lib.types.str; | ||
| }; |
There was a problem hiding this comment.
Are these api keys secrets? If so, please consider using api_key_file since /nix/store is a public place.
There was a problem hiding this comment.
Isn't this limited by chasemapper itself? It has no way to read in an API key file afaik, and this configuration is just used to generate chasemapper's config file (which ends up in /nix/store). So I would think there's not a ton we can do about this
wegank
added
the
2.status: merge conflict
nixpkgs-ci
bot
added
the
12.first-time contribution
nixpkgs-ci
bot
added
the
2.status: stale
6 participants
Add chasemapper, which is a web-app for tracking high-altitude balloons.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.