-
Notifications
You must be signed in to change notification settings - Fork 33
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
Enhancement: eclean-dist should additionally handle git checkouts in $DISTDIR/git3-src #33
Conversation
@floppym Would you mind taking a look? |
pym/gentoolkit/eclean/search.py
Outdated
def git_check(self, distdir): | ||
cpvs = [ | ||
i.split("/")[1][:-5] | ||
for i in set(self.vardb.cpv_all()) | ||
if "live" in self.vardb.aux_get(i, ["PROPERTIES"]) | ||
] | ||
git_src = os.path.join(distdir, "git3-src") | ||
gitdir = dict(map(lambda i: (i.split("_")[1][:-4], i), os.listdir(git_src))) | ||
deprecated_git = [ | ||
os.path.join(git_src, gitdir[i]) for i in gitdir.keys() if i not in cpvs | ||
] | ||
return deprecated_git |
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.
Please add a comment to explain what this is doing. It's very hard to read.
I suspect you are matching directory names in git3-src to ebuild package names. That seems incorrect to me: git-r3.eclass does not use the ebuild package name to calculate the repo_name
variable, which is what determines the directory name within git3-src. The repo_name
variable is calculated based on the git source URI.
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.
Yes @floppym I was doing that now I'm using the env file for the path.
4d8dccf
to
9c0ebf0
Compare
pym/gentoolkit/eclean/search.py
Outdated
] | ||
git_src = os.path.join(distdir, "git3-src") | ||
gitdir = [os.path.join(git_src, i) for i in os.listdir(git_src)] | ||
deprecated_git = [i for i in gitdir if i not in expected_dirs] |
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.
Do you mean obsolete instead of deprecated?
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.
Yes I'll change that
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.
I wonder how this will work for ebuilds that utilize multiple git repos, either directly or indirectly via git submodules. It's probably worth testing that.
It might also be good to give the user a way to opt out of cleaning git repos via a command line option.
pym/gentoolkit/eclean/search.py
Outdated
def git_check(self, distdir): | ||
"""Checks $DISTDIR/git3-src for checkouts which are not in the vardb""" | ||
expected_dirs = [ | ||
self.vardb._aux_env_search(i, ["EGIT_DIR"])["EGIT_DIR"] |
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.
EGIT_DIR contains an absolute path that starts with DISTDIR as it was defined at the time the package was installed.
It is possible for the user to move DISTDIR to another location by copying/moving the files and updating make.conf. In this case, eclean-dist would end up removing all git repos since the paths would not match in the new DISTDIR.
A simple workaround might be to take the last component of EGIT_DIR, and append it to the current value of ${DISTDIR}/git3-src
.
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.
Yes you are right @floppym ! I'll change that
… fetched in the gir3_src see_also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
… fetched in the gir3_src ebuilds which calls git-r3_fetch multiple times for diffrent repos. While EGIT_DIR stores a single repo path, mapping all repos to such packages is currently unfeasible. Introducing GDIR_NAME to address this limitation. see_also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
…eing fetched in the gir3_src ebuilds which calls git-r3_fetch multiple times for diffrent repos. While EGIT_DIR stores a single repo path, mapping all repos to such packages is currently unfeasible. Introducing EGIT_DIR_NAME to address this limitation. see_also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
04154be
to
f36db1e
Compare
@thesamesam @floppym |
efab605
to
3633a71
Compare
@mgorny, please take a look |
449448c
to
d387f4a
Compare
…eing fetched in the gir3_src ebuilds which calls git-r3_fetch multiple times for diffrent repos. While EGIT_DIR stores a single repo path, mapping all repos to such packages is currently unfeasible. Introducing EGIT_DIR_NAME to address this limitation. see_also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
ebuilds which calls git-r3_fetch multiple times for diffrent repos. While EGIT_DIR stores a single repo path, mapping all repos to such packages is currently unfeasible. Introducing EVCS_STORE_DIRS to address this limitation. see_also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
pym/gentoolkit/eclean/search.py
Outdated
.strip("()") | ||
.split() |
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.
This won't handle whitespace correctly. The whole point of using an array is to handle it correctly.
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.
The eclass currently does not use whitespace in repo_name
, so this is problem in theory only.
Are you aware of any way to properly parse a bash array in Python?
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.
The eclass currently does not use whitespace in
repo_name
, so this is problem in theory only.
The variable is supposed to contain the full path.
Are you aware of any way to properly parse a bash array in Python?
Perhaps shlex
would be good enough.
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.
The variable is supposed to contain the full path.
No, it's not. The full path is useless since the user could change DISTDIR at any time.
We can ignore this edge case for now.
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.
Perhaps shlex would be good enough.
Yeah, shlex.split()
will work after stripping the parens.
ebuilds which calls git-r3_fetch multiple times for diffrent repos. While EGIT_DIR stores a single repo path, mapping all repos to such packages is currently unfeasible. Introducing EVCS_STORE_DIRS to address this limitation. see_also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
6e8097d
to
9b1e476
Compare
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.
One more idea: let's make it more "generic", i.e. at least make options about "vcs" rather than "git". While you don't have to immediately support other VCS stores, let's have a single CLI that'll work if someone adds more in the future.
Done |
A new feature for eclean-dist to clean git3-src. Optionally, cleaning the vcs-src can be skipped with --skip-vcs. Bug: https://bugs.gentoo.org/622938 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com>
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.
LGTM from eclasses standpoint. I can't judge all the eclean code though.
ebuilds which calls git-r3_fetch multiple times for diffrent repos. While EGIT_DIR stores a single repo path, mapping all repos to such packages is currently unfeasible. Introducing EVCS_STORE_DIRS to address this limitation. See-Also: gentoo/gentoolkit#33 Signed-off-by: Siddhanth Rathod <xsiddhanthrathod@gmail.com> Closes: #33133 Signed-off-by: Michał Górny <mgorny@gentoo.org>
Thank you! Merged, and appreciate the patience. |
@@ -148,7 +149,7 @@ def findDistfiles( | |||
+ "%s remaining candidates to clean" % len(clean_me) | |||
) | |||
clean_me, saved = self._check_excludes(exclude, clean_me) | |||
return clean_me, saved, deprecated | |||
return clean_me, saved, deprecated, vcs |
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.
This is also needed on line 117 in order to fix https://bugs.gentoo.org/928138.
Bug: https://bugs.gentoo.org/622938