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

autoclean: only enable for global updates and depcleans #795

Closed
wants to merge 1 commit into from

Conversation

thesamesam
Copy link
Member

lib/_emerge/actions.py Outdated Show resolved Hide resolved
@thesamesam thesamesam force-pushed the autoclean branch 3 times, most recently from 15a473d to 559dd9e Compare March 22, 2022 01:13
@thesamesam thesamesam force-pushed the autoclean branch 2 times, most recently from 2fcc776 to 9d81a15 Compare April 10, 2022 11:42
@thesamesam
Copy link
Member Author

@zmedico How should we handle the "AUTOCLEAN is disabled. This can cause serious ..." message? Just drop it now?

@zmedico
Copy link
Member

zmedico commented Apr 10, 2022

@zmedico How should we handle the "AUTOCLEAN is disabled. This can cause serious ..." message? Just drop it now?

I don't think that the autoclean code in vartree.py should be disabled by default, since that really does have dangerous implications (old version is not uninstalled during upgrade). For the purposes of this PR, I think it would make sense to introduce a separate GLOBAL_AUTOCLEAN variable which only affects the global autoclean action that you are trying to optimize.

@thesamesam
Copy link
Member Author

I'm wondering if instead we should just unconditionally autoclean for that case?

@zmedico
Copy link
Member

zmedico commented Apr 10, 2022

Sure, unconditional autoclean in vartree.py seems reasonable. The AUTOCLEAN=n case there really doesn't seem worth supporting.


others_in_slot.append(self) # self has just been merged
for dblnk in list(others_in_slot):
if dblnk is self:
continue
if not (autoclean or dblnk.mycpv == self.mycpv or reinstall_self):
Copy link
Member Author

Choose a reason for hiding this comment

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

When changing to just:

	if not (dblnk.mycpv == self.mycpv or reinstall_self):

this broke the lib/portage/tests/emerge/test_emerge_slot_abi.py test, presumably because something overlapping was left, and normally we were testing with autoclean on.

It passes with removing the condition entirely which I think is what we've agreed?

I've talked myself into circles on whether it's right to drop it or whether the test is the problem, or something else. I think it seems right to drop it: we always want to clean up after ourselves, just not consider all other packages.

@zmedico let me know if I should somehow adapt the test (and how?) if I haven't made the right fix here. It seems right to me to just always autoclean here though like we discussed but I might be missing something (it feels wrong the test broke in the AUTOCLEAN=no case?)

Copy link
Member

Choose a reason for hiding this comment

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

Right, we always want to cleanup after ourselves. The rest of this condition was for the "autoclean is False" case, which no longer exists, so the whole condition is unnecessary now.

Copy link
Member

@zmedico zmedico left a comment

Choose a reason for hiding this comment

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

LGTM

@thesamesam
Copy link
Member Author

Thanks for all the help Zac, really appreciated!

@thesamesam thesamesam deleted the autoclean branch April 13, 2022 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants