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

pymetno: init at 0.5.0 #78132

Merged
merged 2 commits into from Mar 18, 2020
Merged

pymetno: init at 0.5.0 #78132

merged 2 commits into from Mar 18, 2020

Conversation

flyfloh
Copy link
Contributor

@flyfloh flyfloh commented Jan 20, 2020

Another library used by Home-Assistant. This one is for talking to the met.no service and getting the weather.

It seems the tests for aiohttp break when building this for python 3.8, but it works for 3.6 and 3.7.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Comment on lines 14 to 19
src = fetchPypi {
inherit pname version;
sha256 = "e532544495200210407e3d68f719c271435da6f3bfe696e708b1d5d603a21948";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

running build_ext

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK
Finished executing setuptoolsCheckPhase
/nix/store/lkll7y216pk69jl93kizxgmi9hnpv5wy-python3.7-PyMetno-0.5.0

no tests packages in pypi sdist, please fetch from github.

Tests are critical to ensuring the package isn't broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Unfortunately there are not tests on Github either.

Copy link
Contributor

Choose a reason for hiding this comment

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

:(

@jonringer
Copy link
Contributor

unfortunatley, I don't think this is a great candidate for nixpkgs (personal project, no tests), however, https://github.com/nix-community/NUR would be a good candidate if you wanted to share with others.

@Mic92
Copy link
Member

Mic92 commented Jan 28, 2020

Are you sure? It's used within home-assistant, which we have packaged.

@flyfloh
Copy link
Contributor Author

flyfloh commented Feb 4, 2020

I'd be happy to submit a PR to NUR if that's the place to put it. However as @Mic92 pointed out home-assistant depends on this library to get the weather from the met.no service. Wouldn't HA be much harder to use when parts of it depend on NUR?

@jonringer
Copy link
Contributor

I don't currently see it in nixpkgs.

I would be okay with doing pythonImportsCheck on the majority of importable modules. Just want to ensure that it's actually usable when doing dependency bumps. Unit tests are much more preferable as they give some notion of correctness.

@flyfloh
Copy link
Contributor Author

flyfloh commented Feb 28, 2020

I would be okay with doing pythonImportsCheck on the majority of importable modules.

I don't quite get that. What does pythonImportsCheck do? greping for it didn't really enlighten me. Seems like most packages that use it put themselves in there.

Should I do something like this?

pythonImportsCheck = [ "pymetno"];

@jonringer
Copy link
Contributor

pythonImportsCheckPhase () {
    echo "Executing pythonImportsCheckPhase"

    if [ -n "$pythonImportsCheck" ]; then
        echo "Check whether the following modules can be imported: $pythonImportsCheck"
        ( cd $out && eval "@pythonCheckInterpreter@ -c 'import os; import importlib; list(map(lambda mod: importlib.import_module(mod), os.environ[\"pythonImportsCheck\"].split()))'" )
    fi
}

It will essentially do python -c 'import XXXX' to each of the modules you list. This will at least cause the python interpreter to import/evaluate all the modules that it needs at the beginning of the files.

@Mic92
Copy link
Member

Mic92 commented Mar 4, 2020

Let's go with this solution.

@flyfloh
Copy link
Contributor Author

flyfloh commented Mar 9, 2020

I have updated this to use pythonImportsCheck. Thanks for the explanation @jonringer.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

rfc 45, otherwise LGTM'

[3 built, 66 copied (278.0 MiB), 60.6 MiB DL]
https://github.com/NixOS/nixpkgs/pull/78132
1 package built:
python37Packages.pymetno

pkgs/development/python-modules/pymetno/default.nix Outdated Show resolved Hide resolved
Co-Authored-By: Jon <jonringer@users.noreply.github.com>
@Mic92 Mic92 merged commit 57bf255 into NixOS:master Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants