lbl-anp / becquerel

Becquerel is a Python package for analyzing nuclear spectroscopic measurements.
Other
43 stars 16 forks source link

Add liccheck #416

Closed jvavrek closed 4 months ago

jvavrek commented 4 months ago

https://github.com/dhatim/python-license-check

jvavrek commented 4 months ago

Dunno why the pre-commit hook is failing here...

markbandstra commented 4 months ago

I reproduced the CI error on my local machine. To be clear, the error is:

Run Python License Checker...............................................Failed
- hook id: liccheck
- exit code: 1

Executable `liccheck` not found

I noticed in the CI output (and on my machine) that the environment for liccheck is initialized but not installed. I suspect it has to do with the language: system setting, which according to the pre-commit docs:

This hook type will not be given a virtual environment to work with – if it needs additional dependencies the consumer must install them manually.

Removing language: system from the config on my local machine leads to successfully executing liccheck but it raises an error:

pkg_resources.DistributionNotFound: The 'uncertainties>=3.0.3' distribution was not found and is required by the application
jvavrek commented 4 months ago

@markbandstra I think the error you saw just means you don't have uncertainties installed (maybe on your machine, or maybe just in that local environment). Trying the CI with that line removed now...

...and I get it on the CI here.

Strange that I didn't see it on my local machine when the language: system line was still there. I've run into similar issues with some more obscure binaries but chalked that up to non-standard installs. I would expect uncertainties to work fine. We maybe can override it manually in the config file.

jvavrek commented 4 months ago

Dunno, feels like a bug. Specifying uncertainties>=3.0.3 should be no different than black>=22.3.0 or numba>=0.47.0, which both work on the CI.

markbandstra commented 4 months ago

You're right, it's looking in the virtual environment, which does not have uncertainties installed. Looking at setuptools.pkg_resources, it seems like uncertainties is only getting blamed here because of this line in pkg_resources in the WorkingSet.resolve method:

        requirements = list(requirements)[::-1]

so the last requirement in the from requirements.txt is the first one examined and found to be missing.

So it seems like we want the system's liccheck to run and check the system's environment, where the relevant libraries would be installed, so language: system should be correct after all. But how to fix the CI, I do not know. It seems like pip installing requirements-dev.txt should have done the trick.

I think maybe you didn't see it on your local machine because you probably ran pip install liccheck whereas I know I did not, thinking it would only need to exist in a virtual environment.

jvavrek commented 4 months ago

Possibly because there's no pip install requirements.txt (or -dev) in the separate pre-commit job: https://github.com/lbl-anp/becquerel/blob/main/.github/workflows/pre-commit.yaml

jvavrek commented 4 months ago

https://github.com/lbl-anp/becquerel/actions/runs/9304327492/job/25608628469?pr=416

Successfully installed ... uncertainties-3.1.7 ...
pkg_resources.DistributionNotFound: The 'uncertainties>=3.0.3' distribution was not found and is required by the application

🤷

jvavrek commented 4 months ago

Now our pre-commit / run-pre-commit job passes but the automatic pre-commit.ci - pr job fails?

jvavrek commented 4 months ago

As we continue to modernize project configuration, we'll probably move requirements into the pyproject.toml... in that case, we can simplify the list of licenses via regex.