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

[RFC] vlicense hook #21272

Closed
wants to merge 3 commits into from
Closed

[RFC] vlicense hook #21272

wants to merge 3 commits into from

Conversation

sgn
Copy link
Member

@sgn sgn commented Apr 23, 2020

No description provided.

@sgn
Copy link
Member Author

sgn commented Apr 23, 2020

I haven't checked if we need to reset $license_file in subpkg.

@pullmoll
Copy link
Member

I don't see a (big) advantage over specifying vlicense … somewhere in do_install() or post_install(). You may save at most 2 lines in a template. And you cannot handle cases where the license file is created in e.g. post_install() from cropping some README.md, index.html or other source file.

@sgn
Copy link
Member Author

sgn commented Apr 24, 2020 via email

@ahesford
Copy link
Member

ahesford commented Apr 24, 2020

I don't see a (big) advantage over specifying vlicense … somewhere in do_install() or post_install(). You may save at most 2 lines in a template. And you cannot handle cases where the license file is created in e.g. post_install() from cropping some README.md, index.html or other source file.

The license file can still be created in post_install(), if common/hooks/README is accurate; the license hook will run after post_install().

A license_file variable simplifies a very common use case for post_install(). Sure, it only saves two lines, but it makes the template a bit easier to read. (Assuming, of course, that the most common goal in reading a template is not to determine how the license is installed, but how the package is built, installed and packaged.) For more complicated license installations, specific templates can still opt to omit license_file and manually invoke vlicense.

@Vaelatern
Copy link
Member

I want @leahneukirchen to review before this is merged. I personally think the addition of a human readable variable helps in quickly scanning templates for anything weird.

@leahneukirchen
Copy link
Member

I think it cleans up many templates that only need a hook to install the license.

@leahneukirchen
Copy link
Member

I was supposed to review it because I brought up the idea.

@Vaelatern
Copy link
Member

@xtraeme Are you the only one who calls out what seems to be bad behavior? If so, why?

EUNHELPFUL.

@Vaelatern
Copy link
Member

Sounds like someone I know.

This thread is off topic. Are we merging this?

@ahesford
Copy link
Member

+1 to merge

@Chocimier
Copy link
Member

-1 to merge unless it is capable to replace all vlicense usages. It is missing installation of multiple files by glob, like xorgproto or CLion.

@leahneukirchen
Copy link
Member

I also suggest to use license_file="SOURCE>TARGET".

@sgn
Copy link
Member Author

sgn commented Apr 24, 2020 via email

@sgn
Copy link
Member Author

sgn commented Apr 24, 2020 via email

@ahesford
Copy link
Member

I also suggest to use license_file="SOURCE>TARGET".

I second the greater-than syntax. It parallels its use in distfiles to mean "take this and move it here".

@ahesford
Copy link
Member

-1 to merge unless it is capable to replace all vlicense usages. It is missing installation of multiple files by glob, like xorgproto or CLion.

If vlicense remains available, why require that license_files be capable of replacing these more complicated uses? Allowing globs will, as @sgn points out, present some challengs for renaming syntax. Also, they will harm, rather than help, the readability that this change is meant to improve.

I think license_files makes good syntactic sugar to simplify simple license installations; fall back to manual vlicense where complexity demands.

@Vaelatern
Copy link
Member

I think license_files makes good syntactic sugar to simplify simple license installations; fall back to manual vlicense where complexity demands.

So is it too confusing to have two ways to do things? I think not: distfiles works similarly, you can fetch if you must.

@ahesford
Copy link
Member

I think license_files makes good syntactic sugar to simplify simple license installations; fall back to manual vlicense where complexity demands.

So is it too confusing to have two ways to do things? I think not: distfiles works similarly, you can fetch if you must.

No, to the contrary, I think having license_files as an alternative to manual vinstall is a beneficial change. But I don't think the license_files variable needs to handle globs and things. Make each space-separated component of license_files a single file to be installed, possibly with a rename indicated by '>'.

But if it makes sense to do something more complicated like glob matching for installing multiple license files in one pass, leave that for manual work in one of the build/install hooks.

@Chocimier
Copy link
Member

I like that license_files is placed close to license, so its easy to verify, but if we are leave vlicense as option, we'll end up with most of old templates using vlicense, half of new using license_files and half of new using vlicense, i.e. a mess that do not ease reviewing.

I prefer single solution; either staying with vlicense because it works well; or introducing license_files that handles almost all cases, i.e files and globs, banning vlicense and using vinstall (possibly with $LICENSEDIR) for corner cases; over another TIMTOWTDI.

@Chocimier
Copy link
Member

Or maybe we could teach vinstall to install directories, what will be useful on its own, then copy licenses by glob, and put directory into license_files.

@sgn
Copy link
Member Author

sgn commented Apr 26, 2020

Or maybe we could teach vinstall to install directories,
what will be useful on its own, then copy licenses by glob,
and put directory into license_files.

Is this over-complicated?

If we decide to do glob, I'll do it in license_files instead.

@sgn
Copy link
Member Author

sgn commented Apr 30, 2020

Thinking about it now.
I think the glob is too complicated and may not worth our effort.

I think we should add checksum for $license_files instead.
It will force people check if license was changed, it's unusual to change LICENSE file, except bump year, or change license.
This is used by Open Embedded.

@ahesford
Copy link
Member

Is license change a common enough problem that it's worth adding a checksum? The point of license_files is to simplify license installation, not increase the burden.

@Anachron
Copy link
Contributor

I've seen examples of "sed"ed License files from the source. Just a FYI that those might be one of the corner cases.

@sgn
Copy link
Member Author

sgn commented Apr 30, 2020

Is license change a common enough problem that
it's worth adding a checksum? The point of license_files is
to simplify license installation, not increase the burden.

The point is: license should be changed very rarely.
If there's a change in LICENSE, we need to know and update the license field.
Hence, I think adding a checksum makes more sense to me.

@sgn
Copy link
Member Author

sgn commented Apr 30, 2020

I've seen examples of "sed"ed License files from the source.
Just a FYI that those might be one of the corner cases.

Yes, it's one common case, I've made some LICENSE like that.
We could put that sed into post_patch.

@travankor
Copy link
Contributor

I think we should add checksum for $license_files instead.
It will force people check if license was changed, it's unusual to change LICENSE file, except bump year, or change license.
This is used by Open Embedded.

I don't like this approach, personally.
This will make too many false positives if they change contributors or update the year. Additionally, it increases work for all packages for a not 100% reliable detection check.

If there's a change in LICENSE, we need to know and update the license field.
Hence, I think adding a checksum makes more sense to me.

If a project relicenses, this must be noted in the changelog, and updaters have to read the changelog. Relicensing only happens rarely (e.g. Openssl -> Apache, LLVM -> Apache), too.

@travankor
Copy link
Contributor

travankor commented Apr 30, 2020

Thinking about it now.
I think the glob is too complicated and may not worth our effort.

I don't think if its implementation is complicated it is bad. As long as the template is simple for packagers, I am fine. xbps-src and bash have complex implementations already.

@ahesford
Copy link
Member

Is license change a common enough problem that
it's worth adding a checksum? The point of license_files is
to simplify license installation, not increase the burden.

The point is: license should be changed very rarely.
If there's a change in LICENSE, we need to know and update the license field.
Hence, I think adding a checksum makes more sense to me.

The rarity of relicensing militates against adding a checksum. Adding more burden on the packagers just to catch some corner cases doesn't seem to be worthwhile.

Furthermore, this would only be an issue if a project changed the license from something that shouldn't be installed (e.g., Apache-2.0 or one of the GPL versions) to something that should be installed. Most of those cases would be moving away from GPL (6933 templates in void-packages reference GPL; only 702 reference Apache; only 105 reference GFDL), but I suspect (based purely on anecdotes) that almost no projects that adopt the GPL tend to move away from it---license churn seems to happen more among other licenses.

If a package already requires vlicense and changes to another license, the package metadata may be wrong, but the package will still install the correct license on disk.

This seems like a practical non-issue to me.

@ahesford
Copy link
Member

ahesford commented Apr 30, 2020

Thinking about it now.
I think the glob is too complicated and may not worth our effort.

I'm lukewarm on glob support in license_files. Sure, there is some appeal in being able to say

license_files="LICENSE*.txt"

but is it worth complicating the implementation?

If glob support is a must, I certainly wouldn't try to support renames during the globs. Either the matches are copied with their original names, or indiviual files can be listed with source>dest pairs. Trying to batch-rename globs is a recipe for disaster.

@Vaelatern
Copy link
Member

I'd rather the hook be used for simple cases, and vlicense whenever anything gets complicated. Not supporting complicated things that just increase work for all like checksumming or globbing.

@Chocimier
Copy link
Member

Chocimier commented Apr 30, 2020

I'd rather the hook be used for simple cases, and vlicense whenever anything gets complicated.
So is it too confusing to have two ways to do things?

Yes, I think having two ways is too complicated. There will be need to look for license files in two parts of template while reviewing.
People are used to call vlicense, so they will be calling it in new templates: should we accept this? Will existing vlicense calls stay? Hook seem to work with sedding, but is it simple case?

Below is hook mod that handle globs. It fails if no file is matched by glob.

@Chocimier
Copy link
Member

Oh. In fact, hook already handles globs, as they are expanded under for.

@sgn sgn closed this Sep 19, 2020
@sgn sgn deleted the vlicense-hook branch September 19, 2020 03:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants