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

Fix Poetry instructions #11152

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Conversation

ewjoachim
Copy link
Contributor

@ewjoachim ewjoachim commented Feb 25, 2024

Fixes #11150

I believe we will make the (Python) world a better place by setting VIRTUAL_ENV, but this change should be evaluated before breaking other people's build for moving too hastily, so I'm making a first fix.

2 questions:

  • I added a reference to the ticket, because I'm hopeful the VIRTUAL_ENV fix will happen. Is it right?
  • Should we add a warning/notice that these instructions changed in Feb 2024, so that people experiencing the issue know they need to update it?

📚 Documentation previews 📚

@ewjoachim ewjoachim requested a review from a team as a code owner February 25, 2024 22:03
humitos added a commit to readthedocs/test-builds that referenced this pull request Feb 26, 2024
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This solution seems to work fine. I'm happy to merge it.

I did a test at https://beta.readthedocs.org/projects/test-builds/builds/23561155/ and the documentation built properly.

docs/user/build-customization.rst Outdated Show resolved Hide resolved
@humitos
Copy link
Member

humitos commented Feb 26, 2024

I added a reference to the ticket, because I'm hopeful the VIRTUAL_ENV fix will happen. Is it right?

This is a tricky one. Changing all Python builds on Read the Docs behavior is risky and it will have unexpected behaviors for sure. We have experience doing such kind of changes and it usually breaks a good amount of projects. So, I can't guarantee we will be performing this change, unfortunately.

However, I'm happy we have a solution for Poetry that's compatible with its latest version 💯

Should we add a warning/notice that these instructions changed in Feb 2024, so that people experiencing the issue know they need to update it?

I don't have a strong preference here, but I think it's not needed.

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>
@ewjoachim
Copy link
Contributor Author

So, I can't guarantee we will be performing this change, unfortunately.

Yeah, I understand, that's exactly why I asked. So does this means the PR would be better by removing the comment entirely ? If we don't really expect this to change, better not state for all eternity that it will ?

@humitos
Copy link
Member

humitos commented Feb 26, 2024

I don't have a particular preference here. @ericholscher, what do you think?

@agjohnson
Copy link
Contributor

I don't think adding this env var should conflict with how we are executing commands, it feels likely safe for us to add. However, it would also be fairly easy for us to test. I don't think it will affect things much otherwise, as we are explicit about the command path while executing.

@ericholscher
Copy link
Member

I'm pretty sure we don't want to set a new ENV var that has impact on downstream tools if we can avoid it at all. It will definitely break stuff. However, I'm 💯 on adding this approach to our docs.

@agjohnson
Copy link
Contributor

I think it's worth a test as we can't say with any certainty that this will break something otherwise. I hold out hope it's safe, as VIRTUAL_ENV is consumed by venv.

Having a fix in our docs is a fine first step though.

@ewjoachim
Copy link
Contributor Author

I'm not reading any clear answer as to whether the comment:

        # VIRTUAL_ENV needs to be set manually for now.
        # See https://github.com/readthedocs/readthedocs.org/pull/11152/

should appear in the doc or not.

It looks like everyone agrees the rest is fine, my question is mainly on this comment :)

@humitos
Copy link
Member

humitos commented Feb 27, 2024

OK, I will merge this PR as-is since it solves the actual problem and unblock users wanting to use poetry for their dependencies. We can continue discussing the rest in another issue.

@ewjoachim thanks for your contribution and help on this 👍🏼

@humitos humitos merged commit 8eafdaf into readthedocs:main Feb 27, 2024
7 checks passed
@ewjoachim ewjoachim deleted the fix-poetry-instructions branch February 27, 2024 09:02
davidalber added a commit to davidalber/geneagrapher-core that referenced this pull request Feb 27, 2024
teutoburg added a commit to AstarVienna/ScopeSim that referenced this pull request Feb 28, 2024
This follows a change in RTD's user guide found at https://docs.readthedocs.io/en/stable/build-customization.html#install-dependencies-with-poetry, which they changed in readthedocs/readthedocs.org#11152 following a breaking change introduced by Poetry 1.8.
teutoburg added a commit to AstarVienna/skycalc_ipy that referenced this pull request Feb 28, 2024
This follows a change in RTD's user guide found at https://docs.readthedocs.io/en/stable/build-customization.html#install-dependencies-with-poetry, which they changed in readthedocs/readthedocs.org#11152 following a breaking change introduced by Poetry 1.8.
teutoburg added a commit to AstarVienna/speXtra that referenced this pull request Feb 28, 2024
This follows a change in RTD's user guide found at https://docs.readthedocs.io/en/stable/build-customization.html#install-dependencies-with-poetry, which they changed in readthedocs/readthedocs.org#11152 following a breaking change introduced by Poetry 1.8.
matthewfeickert added a commit to scikit-hep/pyhf that referenced this pull request May 10, 2024
* RTD doesn't support uv at the same level as pip, but there are ways
  to still use it along with most of the infrastructure. Adopt the
  strategy provided in ReadTheDocs's 'Build process customization' section
  https://docs.readthedocs.io/en/latest/build-customization.html#install-dependencies-with-uv
  , from readthedocs/readthedocs.org#11152,
  but continue to monitor alternative faster methods such as those
  described in readthedocs/readthedocs.org#11289 .
gjoseph92 added a commit to ZachHoppinen/geogif that referenced this pull request May 21, 2024
openci-bot pushed a commit to TrustedFirmware-A/trusted-firmware-a that referenced this pull request Aug 1, 2024
RTD uses a mixture of poetry and pip to install packages in the build
environment. In the past it was recommended to disable poetry from
creating a fresh virtual environment. Instead, the expectation was that
poetry would be able to detect it's current virtual environment and
install the packages in the right place. This was recently updated to
allow poetry to better allow dependcy management by poetry [1]. Remove
this configuration and explicitly point Poetry to the virtual
environment.

[1] readthedocs/readthedocs.org#11152

Change-Id: I58e49ba6f6d122e70bbcf1dbb10220881a09faf3
Signed-off-by: Harrison Mutai <harrison.mutai@arm.com>
openci-bot pushed a commit to TF-Hafnium/hafnium that referenced this pull request Aug 2, 2024
RTD uses a mixture of poetry and pip to install packages in the build
environment. In the past it was recommended to disable poetry from
creating a fresh virtual environment. Instead, the expectation was that
poetry would be able to detect it's current virtual environment and
install the packages in the right place. This was recently updated to
allow poetry to better allow dependcy management by poetry [1]. Remove
this configuration and explicitly point Poetry to the virtual
environment.

[1] readthedocs/readthedocs.org#11152

Signed-off-by: J-Alves <joao.alves@arm.com>
Signed-off-by: Harrison Mutai <harrison.mutai@arm.com>
Change-Id: I64e75410a0f14ff5edd91ea526d85ad108cb1a13
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.

Builds broken with poetry 1.8
4 participants