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

Check licenses before building rather than after #768

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Feb 21, 2020

If a license is incorrect and will be rejected, it's good to know that before
you've spent time building the project.

As an example, if the license in a Cargo.toml file is wrong and will be
rejected, but we check after the build, you have to rebuild that package even
though none of the source has changed.


Testing done:

I ran a build and check-licenses passed.

I then changed apiserver's Cargo.toml to have license = "Very-Bad-License" and saw it fail immediately, rather than building the whole OS, failing, and then requiring me to rebuild apiserver and the OS images.

I then built with cargo make -e BUILDSYS_ALLOW_FAILED_LICENSE_CHECK=true and saw the license error, but saw the build continue anyway.

@tjkirch tjkirch requested a review from iliana February 21, 2020 21:10
@bcressey
Copy link
Contributor

For local development, I've appreciated the ability to ignore cargo deny failures since we don't really model the expected version as part of our build process, and our integration seems to have changed in backwards-incompatible ways a few times now.

I like the change but I'd like to see a fix for #582 as a fast follow.

@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 21, 2020

For local development, I've appreciated the ability to ignore cargo deny failures

@bcressey - I talked briefly with @iliana about this and we had the idea of setting an environment variable that would throw away the return value of the license check, for development purposes - what do you think?

@bcressey
Copy link
Contributor

bcressey commented Feb 21, 2020

@bcressey - I talked briefly with @iliana about this and we had the idea of setting an environment variable that would throw away the return value of the license check, for development purposes - what do you think?

Would it be off by default, meaning "do not ignore the result"? That could work.

I'd prefer an approach like we have now with BUILDSYS_UPSTREAM_SOURCE_FALLBACK where we fail closed by default.

@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 21, 2020

Would it be off by default, meaning "do not ignore the result"? That could work.

@bcressey Yeah, definitely failing safe. Something like BUILDSYS_ALLOW_FAILED_LICENSE_CHECK=true would be required, or it would fail the build fast. It'd probably include a message about that variable so someone doing local development would know how to skip the error temporarily.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

💼

@tjkirch
Copy link
Contributor Author

tjkirch commented Feb 24, 2020

⬆️ Rebase on develop.
⬇️ Add the ability to override via BUILDSYS_ALLOW_FAILED_LICENSE_CHECK=true as discussed in comments above.

Updated Testing Done above.

If a license is incorrect and will be rejected, it's good to know that before
you've spent time building the project.

As an example, if the license in a Cargo.toml file is wrong and will be
rejected, but we check after the build, you have to rebuild that package even
though none of the source has changed.

This can be overridden by building with
BUILDSYS_ALLOW_FAILED_LICENSE_CHECK=true in case you're doing local development
and don't have license information yet.
@tjkirch tjkirch merged commit 48565fd into develop Feb 24, 2020
@tjkirch tjkirch deleted the licenses-first branch February 24, 2020 21:04
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.

None yet

5 participants