[aur-dev] Should git hook check for missing files?
Hi, I just tested the omittance of an install file after adding an `install` variable to a PKGBUILD and running mksrcinfo. `git push` went on without errors. The same with a local, non-downloadable file. So, my question is: should the git hook also test for missing files? Of course this is non-trivial, as all instances of `source` in .SRCINFO should be processed at that point *and* checked for non-downloadable files (non-URLs). Perhaps it's a feature that could be added in the future. Of course I'm asking because I fear that people will forget to add new patches/install files/etc. to the index and upload commits where necessary files are missing. If you want to test, I created a bogus package at https://aur4.archlinux.org/packages/foobartest/ Best, Marcel
Suggested-by: Marcel Korpel <marcel.lists@gmail.com> Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org> --- scripts/git-integration/git-update.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scripts/git-integration/git-update.py b/scripts/git-integration/git-update.py index 161d42f..0fa1ce2 100755 --- a/scripts/git-integration/git-update.py +++ b/scripts/git-integration/git-update.py @@ -235,6 +235,16 @@ for commit in walker: die_commit('%s field too long: %s' % (field, pkginfo[field]), commit.id) + if 'install' in pkginfo and not pkginfo['install'] in commit.tree: + die_commit('missing install file: %s' % (pkginfo['install']), + commit.id) + + for fname in pkginfo['source']: + if "://" in fname or "lp:" in fname: + continue + if not fname in commit.tree: + die_commit('missing source file: %s' % (fname), commit.id) + srcinfo_raw = repo[repo[sha1_new].tree['.SRCINFO'].id].data.decode() srcinfo_raw = srcinfo_raw.split('\n') srcinfo = aurinfo.ParseAurinfoFromIterable(srcinfo_raw) -- 2.4.1
How does this affect stuff like the different humble bundle games in the aur? You have to download the source and put it in the source directory so I don't think it will be in the commit tree? Thanks Sent from my iPhone
On May 31, 2015, at 12:54 PM, Lukas Fleischer <lfleischer@archlinux.org> wrote:
Suggested-by: Marcel Korpel <marcel.lists@gmail.com> Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org> --- scripts/git-integration/git-update.py | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/scripts/git-integration/git-update.py b/scripts/git-integration/git-update.py index 161d42f..0fa1ce2 100755 --- a/scripts/git-integration/git-update.py +++ b/scripts/git-integration/git-update.py @@ -235,6 +235,16 @@ for commit in walker: die_commit('%s field too long: %s' % (field, pkginfo[field]), commit.id)
+ if 'install' in pkginfo and not pkginfo['install'] in commit.tree: + die_commit('missing install file: %s' % (pkginfo['install']), + commit.id) + + for fname in pkginfo['source']: + if "://" in fname or "lp:" in fname: + continue + if not fname in commit.tree: + die_commit('missing source file: %s' % (fname), commit.id) + srcinfo_raw = repo[repo[sha1_new].tree['.SRCINFO'].id].data.decode() srcinfo_raw = srcinfo_raw.split('\n') srcinfo = aurinfo.ParseAurinfoFromIterable(srcinfo_raw) -- 2.4.1
On Sun, 31 May 2015 at 20:14:22, Daniel Wallace wrote:
How does this affect stuff like the different humble bundle games in the aur?
You have to download the source and put it in the source directory so I don't think it will be in the commit tree? [...]
Good question. There are two options: 1. Drop this patch. 2. Keep this patch and add a dummy file to the affected AUR packages. I am slightly leaning towards option 2. Opinions?
* Lukas Fleischer <lfleischer@archlinux.org> (Sun, 31 May 2015 21:16:09 +0200):
On Sun, 31 May 2015 at 20:14:22, Daniel Wallace wrote:
How does this affect stuff like the different humble bundle games in the aur?
You have to download the source and put it in the source directory so I don't think it will be in the commit tree? [...]
Humble bundle games? I searched for 'humble' but only found one package that might be relevant here. Do you have an example?
Good question. There are two options:
1. Drop this patch. 2. Keep this patch and add a dummy file to the affected AUR packages.
I am slightly leaning towards option 2. Opinions?
First, thanks for the patch (and please use this email address in the future regarding patches, I'm reading lists and whatnot now from this address and only sporadically check the other one using filters, so I might miss messages). Second, I feel more for 2, but I'd like to know how many packages are affected. Best, Marcel
The names usually end in 'hib' instead of humble bundle, standing for Humble Indie Bundle, but there are several other things out there that are similar to this. https://aur.archlinux.org/packages/?O=0&C=0&SeB=nd&K=hib&outdated=&SB=n&SO=a&PP=50&do_Search=Go On Sun, May 31, 2015 at 2:23 PM, Marcel Korpel <marcel.korpel@gmail.com> wrote:
* Lukas Fleischer <lfleischer@archlinux.org> (Sun, 31 May 2015 21:16:09 +0200):
On Sun, 31 May 2015 at 20:14:22, Daniel Wallace wrote:
How does this affect stuff like the different humble bundle games in the aur?
You have to download the source and put it in the source directory so I don't think it will be in the commit tree? [...]
Humble bundle games? I searched for 'humble' but only found one package that might be relevant here. Do you have an example?
Good question. There are two options:
1. Drop this patch. 2. Keep this patch and add a dummy file to the affected AUR packages.
I am slightly leaning towards option 2. Opinions?
First, thanks for the patch (and please use this email address in the future regarding patches, I'm reading lists and whatnot now from this address and only sporadically check the other one using filters, so I might miss messages).
Second, I feel more for 2, but I'd like to know how many packages are affected.
Best, Marcel
Also, I am ok with 2, as long as we get that behavior documented on the wiki page and where the aur guidelines are. On Sun, May 31, 2015 at 2:33 PM, Daniel Wallace <danielwallace@gtmanfred.com
wrote:
The names usually end in 'hib' instead of humble bundle, standing for Humble Indie Bundle, but there are several other things out there that are similar to this.
https://aur.archlinux.org/packages/?O=0&C=0&SeB=nd&K=hib&outdated=&SB=n&SO=a&PP=50&do_Search=Go
On Sun, May 31, 2015 at 2:23 PM, Marcel Korpel <marcel.korpel@gmail.com> wrote:
* Lukas Fleischer <lfleischer@archlinux.org> (Sun, 31 May 2015 21:16:09 +0200):
On Sun, 31 May 2015 at 20:14:22, Daniel Wallace wrote:
How does this affect stuff like the different humble bundle games in the aur?
You have to download the source and put it in the source directory so I don't think it will be in the commit tree? [...]
Humble bundle games? I searched for 'humble' but only found one package that might be relevant here. Do you have an example?
Good question. There are two options:
1. Drop this patch. 2. Keep this patch and add a dummy file to the affected AUR packages.
I am slightly leaning towards option 2. Opinions?
First, thanks for the patch (and please use this email address in the future regarding patches, I'm reading lists and whatnot now from this address and only sporadically check the other one using filters, so I might miss messages).
Second, I feel more for 2, but I'd like to know how many packages are affected.
Best, Marcel
* Daniel Wallace <danielwallace@gtmanfred.com> (Sun, 31 May 2015 14:33:38 -0500):
The names usually end in 'hib' instead of humble bundle, standing for Humble Indie Bundle, but there are several other things out there that are similar to this.
Ah, now I understand. But those packages contain filenames with '://' in it, so I think this patch will just let those PKGBUILDs go through without those files being present, am I right? Nevertheless, there are other cases I didn't think of, like https://aur.archlinux.org/packages/ttf-ms-win8/ where source files should be provided by the user. Providing zero-length dummy files looks like a solution, but isn't, as the checksums provided should be correct against those user-provided files, not the dummy ones. I can't think of a solution to this at the moment, perhaps someone else? And *if* we go for solution 2, it should indeed be well-documented. Best, Marcel
On Sun, 31 May 2015 at 22:09:19, Marcel Korpel wrote:
* Daniel Wallace <danielwallace@gtmanfred.com> (Sun, 31 May 2015 14:33:38 -0500):
The names usually end in 'hib' instead of humble bundle, standing for Humble Indie Bundle, but there are several other things out there that are similar to this.
Ah, now I understand. But those packages contain filenames with '://' in it, so I think this patch will just let those PKGBUILDs go through without those files being present, am I right?
Nevertheless, there are other cases I didn't think of, like https://aur.archlinux.org/packages/ttf-ms-win8/ where source files should be provided by the user. Providing zero-length dummy files looks like a solution, but isn't, as the checksums provided should be correct against those user-provided files, not the dummy ones.
I can't think of a solution to this at the moment, perhaps someone else?
Why are checksums an issue? You can use the checksum of the correct file. It doesn't match the checksum of the dummy file but I don't see how that is an issue (it is even good since the user immediately notices that something is wrong with the dummy file). Another possibility is to tell makepkg to skip the integrity check.
And *if* we go for solution 2, it should indeed be well-documented.
Best, Marcel
On Sun, 2015-05-31 at 22:49 +0200, Lukas Fleischer wrote:
On Sun, 31 May 2015 at 22:09:19, Marcel Korpel wrote:
* Daniel Wallace <danielwallace@gtmanfred.com> (Sun, 31 May 2015 14:33:38 -0500):
The names usually end in 'hib' instead of humble bundle, standing for Humble Indie Bundle, but there are several other things out there that are similar to this.
Ah, now I understand. But those packages contain filenames with '://' in it, so I think this patch will just let those PKGBUILDs go through without those files being present, am I right?
Nevertheless, there are other cases I didn't think of, like https://aur.archlinux.org/packages/ttf-ms-win8/ where source files should be provided by the user. Providing zero-length dummy files looks like a solution, but isn't, as the checksums provided should be correct against those user-provided files, not the dummy ones.
I can't think of a solution to this at the moment, perhaps someone else?
Why are checksums an issue? You can use the checksum of the correct file. It doesn't match the checksum of the dummy file but I don't see how that is an issue (it is even good since the user immediately notices that something is wrong with the dummy file). Another possibility is to tell makepkg to skip the integrity check.
And *if* we go for solution 2, it should indeed be well-documented.
Best, Marcel
I am against dummy files and would even prefer dropping the patch in favor of a clean processing of files. Correct me if I am wrong but since the information is extracted from the .SRCINFO file, the package ttf-m-win8 should work just fine. The only problem is which files are delivered and which shell be downloaded. As things stand right now everything with " ://" or "lp:" in its filename is considered an URL and therefore the present of the file is not checked. This would potentially ignore cases where those files are omitted though not downloadable. However considering that this will help the vast majority where this schema fits, the minority of missing warnings are neglectable.
On Mon, 01 Jun 2015 at 00:00:31, Gordian Edenhofer wrote:
[...]
Why are checksums an issue? You can use the checksum of the correct file. It doesn't match the checksum of the dummy file but I don't see how that is an issue (it is even good since the user immediately notices that something is wrong with the dummy file). Another possibility is to tell makepkg to skip the integrity check.
And *if* we go for solution 2, it should indeed be well-documented.
Best, Marcel
I am against dummy files and would even prefer dropping the patch in favor of a clean processing of files.
What do you mean by "clean processing of files"? I consider the version with plausibility checks to be cleaner than the version without.
Correct me if I am wrong but since the information is extracted from the .SRCINFO file, the package ttf-m-win8 should work just fine. The only problem is which files are delivered and which shell be downloaded. As things stand right now everything with " ://" or "lp:" in its filename is considered an URL and therefore the present of the file is not checked. This would potentially ignore cases where those files are omitted though not downloadable. However considering that this will help the vast majority where this schema fits, the minority of missing warnings are neglectable.
Sorry, but I do not follow your argument. The patch uses the same mechanism as makepkg to check whether a file is local.
On Mon, 2015-06-01 at 00:25 +0200, Lukas Fleischer wrote:
On Mon, 01 Jun 2015 at 00:00:31, Gordian Edenhofer wrote:
[...]
Why are checksums an issue? You can use the checksum of the correct file. It doesn't match the checksum of the dummy file but I don't see how that is an issue (it is even good since the user immediately notices that something is wrong with the dummy file). Another possibility is to tell makepkg to skip the integrity check.
And *if* we go for solution 2, it should indeed be well -documented.
Best, Marcel
I am against dummy files and would even prefer dropping the patch in favor of a clean processing of files.
What do you mean by "clean processing of files"? I consider the version with plausibility checks to be cleaner than the version without.
Basically I was thinking about the exact same problem as you were writing about in your recent mail: It is somewhat inconvenient to package files like this when considering the integrity check done by makepkage. Having a dummy file in the git repo and the true file for the checksum is somewhat odd. Using SKIP is also not a true alternative IMHO.
Correct me if I am wrong but since the information is extracted from the .SRCINFO file, the package ttf-m-win8 should work just fine. The only problem is which files are delivered and which shell be downloaded. As
Just ignore this part, I misunderstood previous mails. Sorry about that.
things stand right now everything with " ://" or "lp:" in its filename is considered an URL and therefore the present of the file is not checked. This would potentially ignore cases where those files are omitted though not downloadable. However considering that this will help the vast majority where this schema fits, the minority of missing warnings are neglectable.
Sorry, but I do not follow your argument. The patch uses the same mechanism as makepkg to check whether a file is local.
This kind of check would see files which are named e.g. lp:foo as not missing even if they are.
On Mon, 01 Jun 2015 at 00:48:23, Gordian Edenhofer wrote:
[...] Having a dummy file in the git repo and the true file for the checksum is somewhat odd. Using SKIP is also not a true alternative IMHO.
True. Missing source files are somewhat odd, too, though. You could argue that pretending to have those files is even more odd, but as you already mentioned, only a very small number of packages is affected. The benefit of reducing the number of forgotten files in all other packages is likely to be much higher than the advantage of not having to use dummy files.
[...] This kind of check would see files which are named e.g. lp:foo as not missing even if they are.
Sure, we only check local files and lp:foo is a remote file. Checking remote sources is something that we cannot do properly.
[...] This kind of check would see files which are named e.g. lp:foo as not missing even if they are.
Sure, we only check local files and lp:foo is a remote file. Checking remote sources is something that we cannot do properly.
I did not suggest checking remote sources. My question was why excluding the check for files with "lp:" in their name. lp:whatever is a valid name for a local file, why should I expect them to be remote?
On Mon, 01 Jun 2015 at 15:09:43, Gordian Edenhofer wrote:
[...] This kind of check would see files which are named e.g. lp:foo as not missing even if they are.
Sure, we only check local files and lp:foo is a remote file. Checking remote sources is something that we cannot do properly.
I did not suggest checking remote sources. My question was why excluding the check for files with "lp:" in their name. lp:whatever is a valid name for a local file, why should I expect them to be remote?
"lp:whatever" is not a valid name for a local file in makepkg(8). In order to avoid confusion with Bazaar/Launchpad URIs, everything that contains "lp:" is considered a remote file. You can check the makepkg source code to convince yourself that it uses the same method for distinguishing between local and remote files. [1] https://projects.archlinux.org/pacman.git/tree/scripts/libmakepkg/util/sourc...
On Mon, 2015-06-01 at 15:34 +0200, Lukas Fleischer wrote:
On Mon, 01 Jun 2015 at 15:09:43, Gordian Edenhofer wrote:
[...] This kind of check would see files which are named e.g. lp:foo as not missing even if they are.
Sure, we only check local files and lp:foo is a remote file. Checking remote sources is something that we cannot do properly.
I did not suggest checking remote sources. My question was why excluding the check for files with "lp:" in their name. lp:whatever is a valid name for a local file, why should I expect them to be remote?
"lp:whatever" is not a valid name for a local file in makepkg(8). In order to avoid confusion with Bazaar/Launchpad URIs, everything that contains "lp:" is considered a remote file. You can check the makepkg source code to convince yourself that it uses the same method for distinguishing between local and remote files.
[1] https://projects.archlinux.org/pacman.git/tree/scripts/libmakepkg /util/source.sh.in#n40
Thank you very much for the explanation! I was not aware of that.
* Lukas Fleischer <lfleischer@archlinux.org> (Sun, 31 May 2015 22:49:19 +0200):
On Sun, 31 May 2015 at 22:09:19, Marcel Korpel wrote:
Nevertheless, there are other cases I didn't think of, like https://aur.archlinux.org/packages/ttf-ms-win8/ where source files should be provided by the user. Providing zero-length dummy files looks like a solution, but isn't, as the checksums provided should be correct against those user-provided files, not the dummy ones.
I can't think of a solution to this at the moment, perhaps someone else?
Why are checksums an issue? You can use the checksum of the correct file. It doesn't match the checksum of the dummy file but I don't see how that is an issue (it is even good since the user immediately notices that something is wrong with the dummy file). Another possibility is to tell makepkg to skip the integrity check.
Isn't there a check of checksums before files are added to the index (through mksrcinfo/makepkg)? If that's not the case, I stand corrected.
On Mon, 01 Jun 2015 at 00:07:58, Marcel Korpel wrote:
* Lukas Fleischer <lfleischer@archlinux.org> (Sun, 31 May 2015 22:49:19 +0200):
On Sun, 31 May 2015 at 22:09:19, Marcel Korpel wrote:
Nevertheless, there are other cases I didn't think of, like https://aur.archlinux.org/packages/ttf-ms-win8/ where source files should be provided by the user. Providing zero-length dummy files looks like a solution, but isn't, as the checksums provided should be correct against those user-provided files, not the dummy ones.
I can't think of a solution to this at the moment, perhaps someone else?
Why are checksums an issue? You can use the checksum of the correct file. It doesn't match the checksum of the dummy file but I don't see how that is an issue (it is even good since the user immediately notices that something is wrong with the dummy file). Another possibility is to tell makepkg to skip the integrity check.
Isn't there a check of checksums before files are added to the index (through mksrcinfo/makepkg)? If that's not the case, I stand corrected.
Right, I forgot about that. So the two possibilities left are (a) Build the package using the original files, replace with dummy files before committing. (b) Use "SKIP" for the checksums. I realize that (a) might be a bit cumbersome but then again, users currently need to fix their source tarballs manually after running `makepkg --source` (and remove some source files) which is cumbersome as well.
* Lukas Fleischer <lfleischer@archlinux.org> (Mon, 01 Jun 2015 00:28:56 +0200):
On Mon, 01 Jun 2015 at 00:07:58, Marcel Korpel wrote:
Isn't there a check of checksums before files are added to the index (through mksrcinfo/makepkg)? If that's not the case, I stand corrected.
Right, I forgot about that. So the two possibilities left are
(a) Build the package using the original files, replace with dummy files before committing.
(b) Use "SKIP" for the checksums.
I realize that (a) might be a bit cumbersome but then again, users currently need to fix their source tarballs manually after running `makepkg --source` (and remove some source files) which is cumbersome as well.
I slept over it and thought that (a) is the best way to go, as maintainers have to do something like that now (and it's only an edge case that doesn't occur too much). Skipping integrity checks is really awful, especially when binaries are downloaded, like in https://aur.archlinux.org/packages/mathematica (but the maintainer of that package uses the file: protocol, also a clever workaround, I think; perhaps that's a thing that could be added to the wiki?).
Am 31.05.2015 um 20:14 schrieb Daniel Wallace:
How does this affect stuff like the different humble bundle games in the aur?
You have to download the source and put it in the source directory so I don't think it will be in the commit tree?
Thanks
Sent from my iPhone
On May 31, 2015, at 12:54 PM, Lukas Fleischer <lfleischer@archlinux.org> wrote:
Suggested-by: Marcel Korpel <marcel.lists@gmail.com> Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org> --- scripts/git-integration/git-update.py | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/scripts/git-integration/git-update.py b/scripts/git-integration/git-update.py index 161d42f..0fa1ce2 100755 --- a/scripts/git-integration/git-update.py +++ b/scripts/git-integration/git-update.py @@ -235,6 +235,16 @@ for commit in walker: die_commit('%s field too long: %s' % (field, pkginfo[field]), commit.id)
+ if 'install' in pkginfo and not pkginfo['install'] in commit.tree: + die_commit('missing install file: %s' % (pkginfo['install']), + commit.id) + + for fname in pkginfo['source']: + if "://" in fname or "lp:" in fname: + continue + if not fname in commit.tree: + die_commit('missing source file: %s' % (fname), commit.id) + srcinfo_raw = repo[repo[sha1_new].tree['.SRCINFO'].id].data.decode() srcinfo_raw = srcinfo_raw.split('\n') srcinfo = aurinfo.ParseAurinfoFromIterable(srcinfo_raw) -- 2.4.1
Mostly maintainers use pseudo URLs[1] like: * hib:// or hb:// for the humble bundle stuff * gog:// or gogdownloader:// for gog.com games * local:// or file:// for files you need to download yourself With a custom download agent and a fallback message, this has worked for quite some time. Following this scheme, there should be no problem even with this patch. Maybe we can (mis)use local:// as a general solution, at least for files where no specific DLAGENT is available. About the patch: I think changelog[2] files should be checked also, even if they are not common (anymore). Maybe they will be deprecated soon, but at least now they are used by 778[3] packages. best regards, carstene1ns [1]: https://paste.xinu.at/2YKm1/ (grep'ed through the aur-mirror, list not filtered for false positives - for example in description field) [2]: https://www.archlinux.org/pacman/PKGBUILD.5.html#_options_and_directives [3]: https://paste.xinu.at/blblaK/ (grep'ed through the aur-mirror)
Suggested-by: Marcel Korpel <marcel.korpel@gmail.com> Suggested-by: carstene1ns <arch@carsten-teibes.de> Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org> --- scripts/git-integration/git-update.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/scripts/git-integration/git-update.py b/scripts/git-integration/git-update.py index 2c15912..c7d64df 100755 --- a/scripts/git-integration/git-update.py +++ b/scripts/git-integration/git-update.py @@ -236,6 +236,17 @@ for commit in walker: die_commit('%s field too long: %s' % (field, pkginfo[field]), commit.id) + for field in ('install', 'changelog'): + if field in pkginfo and not pkginfo[field] in commit.tree: + die_commit('missing %s file: %s' % (field, pkginfo[field]), + commit.id) + + for fname in pkginfo['source']: + if "://" in fname or "lp:" in fname: + continue + if not fname in commit.tree: + die_commit('missing source file: %s' % (fname), commit.id) + srcinfo_raw = repo[repo[sha1_new].tree['.SRCINFO'].id].data.decode() srcinfo_raw = srcinfo_raw.split('\n') srcinfo = aurinfo.ParseAurinfoFromIterable(srcinfo_raw) -- 2.4.2
participants (5)
-
carstene1ns
-
Daniel Wallace
-
Gordian Edenhofer
-
Lukas Fleischer
-
Marcel Korpel