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

Add liccheck #416

Merged
merged 10 commits into from
Jun 5, 2024
Merged

Add liccheck #416

merged 10 commits into from
Jun 5, 2024

Conversation

jvavrek
Copy link
Contributor

@jvavrek jvavrek commented May 29, 2024

@jvavrek
Copy link
Contributor Author

jvavrek commented May 29, 2024

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

@markbandstra
Copy link
Member

markbandstra commented May 30, 2024

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
Copy link
Contributor Author

jvavrek commented May 30, 2024

@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
Copy link
Contributor Author

jvavrek commented May 30, 2024

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
Copy link
Member

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
Copy link
Contributor Author

jvavrek commented May 30, 2024

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
Copy link
Contributor Author

jvavrek commented May 30, 2024

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

🤷

Copy link
Member

@markbandstra markbandstra left a comment

Choose a reason for hiding this comment

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

Looks good, good idea!

@jvavrek
Copy link
Contributor Author

jvavrek commented Jun 5, 2024

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

@jvavrek jvavrek self-assigned this Jun 5, 2024
@jvavrek jvavrek merged commit fc39bb4 into main Jun 5, 2024
21 of 22 checks passed
@jvavrek jvavrek deleted the add-liccheck branch June 5, 2024 22:25
@jvavrek
Copy link
Contributor Author

jvavrek commented Jun 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants