build: Clear out stale dependencies when installing requirements - #31080
Merged
Conversation
timmc-edx
force-pushed
the
timmc/ci-sync
branch
from
October 3, 2022 17:05
d042fd1 to
7ef4ce6
Compare
This should help prevent some cross-version caching issues in the edx-platform-runner unit tests, which apparently run on a "dirty" environment. (This results in errors between master and older open-releases but could also prevent us from noticing missing deps.) Calling `make local-requirements` at the end of each `*-requirements` target rather than making it a prerequisite is necessary for using sync, since otherwise the local reqs would be wiped out. Adding a prerequisite of `pre-requirements` allows us to simplify some workflow calls slightly. The pip lockfile also wasn't being used in the Makefile, so fix that. Also fix leading whitespace issue in Makefile.
timmc-edx
force-pushed
the
timmc/ci-sync
branch
from
October 3, 2022 18:05
7ef4ce6 to
98cbd68
Compare
timmc-edx
commented
Oct 3, 2022
|
|
||
| - name: Install Required Python Dependencies | ||
| run: | | ||
| pip install -r requirements/pip.txt |
Contributor
Author
There was a problem hiding this comment.
All of these removals are just to take advantage of the Make targets now doing this automatically.
timmc-edx
commented
Oct 3, 2022
kdmccormick
reviewed
Oct 3, 2022
kdmccormick
left a comment
kdmccormick
left a comment
Member
There was a problem hiding this comment.
one request, otherwise looks like a great change.
BTW, all these X-requirements targets are brand new so don't worry too much about messaging out changes to them.
They were basically doing the same thing, except the newer `dev-requirements` didn't include private reqs.
kdmccormick
approved these changes
Oct 3, 2022
kdmccormick
left a comment
kdmccormick
left a comment
Member
There was a problem hiding this comment.
One non-blocking nit, otherwise 🚀
Contributor
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
Contributor
|
EdX Release Notice: This PR has been deployed to the production environment. |
timmc-edx
added a commit
that referenced
this pull request
Oct 3, 2022
…ents The comment starting at column zero made this Action unparseable. But the comment can also just be removed entirely by using the new `dev-requirements` target, which after PR #31080/commit 2edbdcf now also installs pip and edxapp. I'm not exactly sure why `--src` is being specified here (is this a global package installation, and therefore the editable deps would be installed in the working dir?) but it can be specified even with pip-sync via `PIP_SRC`. I also added it to the verify job since it should probably be there as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should help prevent some cross-version caching issues in the edx-platform-runner unit tests, which apparently run on a "dirty" environment. (This results in errors between master and older open-releases but could also prevent us from noticing missing deps.)
Calling
make local-requirementsat the end of each*-requirementstarget rather than making it a prerequisite is necessary for using sync, since otherwise the local reqs would be wiped out.Adding a prerequisite of
pre-requirementsallows us to simplify some workflow calls slightly.Also fix leading whitespace issue in Makefile.
We may wish to backport some or all of these changes to Nutmeg or maybe even Maple due to the caching issues that led to this PR (see discussion in #30759) but the changes will almost certainly not apply cleanly, and will have to be replayed by hand.