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

docker: Actually install dependencies #32

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

wjt
Copy link
Member

@wjt wjt commented Sep 5, 2024

Previously, pipenv install would relock dependencies unless you
passed --ignore-pipfile. This was changed in pipenv 2024.0.0:

As much requested, the install no longer does a complete lock
operation. Instead install follows the same code path as pipenv update
(which is upgrade + sync).
https://github.com/pypa/pipenv/blob/v2024.0.0/CHANGELOG.md#behavior-changes

What does not seem to be documented is that, if no Pipfile is present,
pipenv install now does nothing.

Adjust for this: add the Pipfile to the container as well as
Pipfile.lock, and drop --ignore-pipfile.

@wjt wjt requested a review from dbnicholson September 5, 2024 12:40
@wjt wjt marked this pull request as ready for review September 5, 2024 12:40
@wjt
Copy link
Member Author

wjt commented Sep 5, 2024

Weirdly on b8803b4 the CI passed. Then the next commit, c9fed63, which I pushed directly to master this morning (note to self: never do this even if it's Obviously Safe) failed. It's weird that pipenv changed in this way. I think pypa/pipenv#6098 is the change.

Reading the news entries a little more i think this change is wrong. One moment...

@wjt wjt marked this pull request as draft September 5, 2024 12:45
@wjt wjt force-pushed the docker-fix-pipenv-install branch from 4a7b604 to 784e154 Compare September 5, 2024 12:50
@wjt wjt changed the title Docker: Install dependencies using pipenv sync docker: Actually install dependencies Sep 5, 2024
@wjt wjt marked this pull request as ready for review September 5, 2024 13:09
Dockerfile Outdated

RUN pipenv install --ignore-pipfile --dev
RUN pipenv install --dev
Copy link
Member

Choose a reason for hiding this comment

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

What you really want to do is call pipenv sync. Then you still only need Pipfile.lock. I thought I made that change here, but I only did it in Azafea.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did in the first version of this patch. If you read the diff in https://github.com/pypa/pipenv/pull/6098/files they changed https://pipenv.pypa.io/en/latest/workflows.html from recommending pipenv sync to pipenv install. I think both would work. OK, I'll go back to v1.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't read the earlier context. That is... quite the choice they made there. How did all those people involved in the project review that PR and decide that changing the behavior of install again was OK?

IMO, I think it's better to stick with sync since it clearly does what we want with both older and newer versions of pipenv.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably start pinning the version of pipenv used since clearly the maintainers have little regard for compatibility. I also like the "we're using semantic versioning now but the major version is just the year" policy. Uh, ok.

Previously, `pipenv install` would relock dependencies unless you
passed `--ignore-pipfile`. This was changed in pipenv 2024.0.0:

> As much requested, the `install` no longer does a complete lock
> operation. Instead `install` follows the same code path as pipenv update
> (which is upgrade + sync).
> – https://github.com/pypa/pipenv/blob/v2024.0.0/CHANGELOG.md#behavior-changes

What does not seem to be documented is that, if no `Pipfile` is present,
`pipenv install` now does nothing.

Use `pipenv sync` instead.
@wjt wjt force-pushed the docker-fix-pipenv-install branch from 784e154 to cf81aa5 Compare September 5, 2024 15:19
@wjt wjt requested a review from dbnicholson September 5, 2024 15:53
@wjt wjt merged commit f7c5a13 into master Sep 5, 2024
4 checks passed
@wjt wjt deleted the docker-fix-pipenv-install branch September 5, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants