-
Notifications
You must be signed in to change notification settings - Fork 293
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
Test for invalid repo_depends items (closes #1414) #1574
Conversation
TODO if this PR is accepted: reject in-official-repos and orphaning issues in issuebot [1] if the to-be-removed package is listed in repo_depends of another package. [1] https://github.com/archlinuxcn/misc_scripts/blob/master/issuebot |
To read |
For now it does not work as
And as you know, not everyone agreed on migrating
Do you mean something like the commit 77660f3 in #979? I'm fine if such a commit is the only stuff to add. A bigger issue is that one needs to replicate complex logic to handle I guess you want to make the new test run no matter a packager clones lilac or not? An approach might be vendoring necessary files from lilac2 and winterpy and adjust |
On Sun, Apr 12, 2020 at 01:17:48AM -0700, Chih-Hsuan Yen wrote:
> you can load only `lilac.yaml`
For now it does not work as
```
$ rg -lg lilac.py repo_depends archlinuxcn | wc -l
23
```
And as you know, not everyone agreed on migrating `repo_depends` to `lilac.yaml` (#990).
Sigh... We should port them. lilac.yaml is there to enable wider and
safer scripting, which is exactly what a check script needs.
> hack the importing system to return fake modules
Do you mean something like the commit 77660f3 in #979? I'm fine if such a commit is the only stuff to add. A bigger issue is that one needs to replicate complex logic to handle `lilac.py` and `lilac.yaml`, like `_parse_lilac`, `_parse_lilac_py` and `_parse_lilac_yaml` functions in #979, or `lilacpy.py` and `lilacyaml.py` in lilac2. I'm afraid that will be a big maintenance burden.
3rd party modules are usually used inside functions so if we don't run
the functions, and provide a fake module that can be imported, it should
work. We can hook into the importing system and turn every unknown
module to a fake one.
But this is not a good way to go.
I guess you want to make the new test run no matter a packager clones lilac or not? An approach might be vendoring necessary files from lilac2 and winterpy and adjust `sys.path` in pre-commit. I can create a script for that if that sound fine to you.
No, we should "vendor" the needed data.
In fact, sometimes I am thinking about move more data from PKGBUILD into
a declarative representation. That's to say, use a TOML file to specify
package name, dependencies, source URLs, etc, so we could have more
metadata to use, fast, safely and without bashes.
…--
Best regards,
lilydjwg
|
So, let's go back to #990 first? |
Rebased after #1615. I also added a check to deny |
python-lhafile is now in [community]. See #1313
superlu is now in [community]. See #1678
Use newer Python as the original version of this patch uses lilac, and the latter has `from __future__ import annotations`, which is not available until Python 3.7. Now newer Python is not neccessary, but it is still better to upgrade sooner than later. And repo_depends are now denied in lilac.py (#990)
Rebased and tested manually. A new broken package Given that in-official-repos and orphaned packages are now automatically handled by the issuebot, could this be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mostly looks good to me. @farseerfc what do you think?
lilac_yaml = yaml.load(lilac_yaml_file.read(), Loader=yaml.SafeLoader) | ||
for dep in lilac_yaml.get('repo_depends', []): | ||
if isinstance(dep, dict): | ||
dep_package = list(dep.keys())[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of reading this line is that, this only checks pkgbase name for splitpkgs, which is fine for solving #1414 and should work in most cases (the number of repo_depends: - pkgbase: pkgname
is limited). Maybe we should leave a note about this for future improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added a not about this line in the new commit.
LGTM except for the note on splitpkgs name. |
Should I merge it and make the change about list_packages function name? |
Marked as draft as this depends on archlinuxcn/lilac#144. Will update after that pull request is merged or another approach is proposed.
See https://travis-ci.org/github/yan12125/repo/builds/673671061 for an example failing test.