Sort activation order of the activation scripts #7176
Conversation
|
cc @mingwandroid , @msarahan |
| return self.path_conversion(sorted(glob(join( | ||
| prefix, 'etc', 'conda', 'activate.d', '*' + self.script_extension | ||
| ))) | ||
| )))) |
There was a problem hiding this comment.
Let's keep all the logic in this helper method. So prefer this over the change on line 173.
There was a problem hiding this comment.
Agreed and done.
kalefranz
left a comment
There was a problem hiding this comment.
I didn't realize glob() returned in random order. This is a good catch.
conda/activate.py
Outdated
| )))) | ||
|
|
||
| def _get_deactivate_scripts(self, prefix): | ||
| return self.path_conversion(glob(join( |
There was a problem hiding this comment.
We should also do the same thing here. Except to be technically correct, it should be sorted() with a reverse=True flag.
|
The test failures are a list vs tuple mismatch. I think if you remove the line 173 change we should be good on those specific test failures. |
|
I think sorted alphanumerically is better than random, but I also think topologically sorting first then alphanumerically after is best. |
|
Yeah I agree we need to sort topologically too. That’ll take additional work. The intent in 4.4 was to sort basically based on an ascii table.
…Sent from my iPhone
On Apr 17, 2018, at 5:17 PM, Ray Donnelly ***@***.***> wrote:
I think sorted alphanumerically is better than random, but I also think topologically sorting first then alphanumerically after is best.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This reverts commit fd3786b.
|
Anything else needs to be done here? When can we expect this to go in? |
|
Is there an issue for using topo sort for this? |
|
@jakirkham The launch point there is #5082 |
|
xref: #6338 (comment) |
|
Hi there, thank you for your contribution to Conda! This pull request has been automatically locked since it has not had recent activity after it was closed. Please open a new issue or pull request if needed. |
Currently, the activation script paths are gathered with
glob.globwhich returns its results in arbitrary order. AFAICT the resulting results are not sorted or manipulated anywhere else. This caused us problems when we added an activation script to a metapackage of ours where we wanted to manipulate the environment variables set by the new compilers, ContinuumIO/anaconda-issues#9172. This PR sorts the resulting list of activation scripts. This way packages can ensure when their activation scripts are run by naming them accordingly. As @msarahan suggested, a script namedzz_patch_flags.shwill run after compiler scripts that areactivate-*.sh.Here, sorting is done in two places, but one of them is redundant. I am submitting both of them, but will remove one upon feedback.