Read Poetry version from poetry.lock

[jacebrowning]

Newer versions of Poetry include the version that was used to create the poetry.lock file as a comment in the first line:

# This file is automatically @generated by Poetry 1.5.1 and should not be changed by hand.

Using that information, the buildpack can infer the version of Poetry that needs to be installed.

[zyv]

@jacebrowning this is actually a cool idea, my understanding is that you propose the following:

1. If config var is set, it takes precedence
2. If not, we could try using the version from the lock file if it's there
3. Otherwise fallback to the hardcoded version

Could you please still

1. Fix the warning
2. Add a test
3. Update the README

Thanks!

[jacebrowning]

@zyv your understanding is correct!

Is there a way to get the lint-and-test job to run after each commit so I can be sure I've fixed all warnings and tests?

[zyv]

Is there a way to get the lint-and-test job to run after each commit so I can be sure I've fixed all warnings and tests?

I have approved the workflow after you have submitted your PR, so actually it should run after each commit, and it seems that it does so.

The warning is fixed however you achieved that (unfortunately, didn't have time to check it out yet), but now (predictably) the tests are failing.

[jacebrowning]

@zyv I see "workflow requires approval" when I push a new commit:

[zyv]

@zyv I see "workflow requires approval" when I push a new commit:

Well, that's weird, I guess that I will have to approve the workflows in the PR until it's merged and you become a second-time contributor :) I thought that one approval per PR with these settings is enough.

However, you should be able to run these workflows in your own fork without any limitations, and of course also run the tests locally. This is how we develop this buildpack.

[jacebrowning]

@zyv I can't seem to get the tests to run locally, even from the main branch:

➜  python-poetry-buildpack git:(master) bash run_tests.sh
1..18
ok - compile sanity-1
ok - compile sanity-2
ok - compile sanity-2
not ok - compile no_vars_success
# --- fixtures/compile-no_vars_success.stdout.txt	2023-08-19 12:40:11
# +++ /var/folders/h1/2b6gts3x37zcnj5khyv8y5180000gn/T/tmp.5kJlABk2/compile-no_vars_success.stdout.txt	2023-08-26 00:18:09
# @@ -7,4 +7,3 @@
#         >>> mocked poetry call <<<
#  -----> Export Python version from Poetry to Heroku runtime.txt file
#  -----> Read Python version from poetry.lock
# ------> Write 3.8.3 into runtime.txt
# --- fixtures/compile-no_vars_success.stderr.txt	2023-08-19 12:40:11
# +++ /var/folders/h1/2b6gts3x37zcnj5khyv8y5180000gn/T/tmp.5kJlABk2/compile-no_vars_success.stderr.txt	2023-08-26 00:18:09
# @@ -0,0 +1 @@
# +----->  is not valid, please specify an exact Python version (e.g. 3.8.1 or ==3.8.1) in your pyproject.toml (and thus poetry.lock)

So, I'm afraid I have to abandon this proposal for now, but if someone with more context on this project wants to add this feature it would be much appreciated.

[zyv]

@zyv I can't seem to get the tests to run locally, even from the main branch:

Hmmm, this is very weird. So what is the platform that you are running the tests on? I'm developing on Ubuntu 20.04, and GitHub runners have Ubuntu 22.04 - both seem to be working fine.

From your code it looks like as if you were running the tests on macOS. I've tried to switch from bash regex test to portable expr in the commit I pushed to this PR, maybe this would solve the issue?

[jacebrowning]

I'm running macOS 13.4.1 and I can't seem to get Python 3.8.1 installed, which seems to be required for the tests:

bash run_tests.sh
1..18
ok - compile sanity-1
ok - compile sanity-2
ok - compile sanity-2
not ok - compile no_vars_success
# --- fixtures/compile-no_vars_success.stdout.txt	2023-08-19 12:40:11
# +++ /var/folders/h1/2b6gts3x37zcnj5khyv8y5180000gn/T/tmp.Gz9LFK18/compile-no_vars_success.stdout.txt	2023-08-28 15:16:09
# @@ -7,4 +7,3 @@
#         >>> mocked poetry call <<<
#  -----> Export Python version from Poetry to Heroku runtime.txt file
#  -----> Read Python version from poetry.lock
# ------> Write 3.8.3 into runtime.txt
# --- fixtures/compile-no_vars_success.stderr.txt	2023-08-19 12:40:11
# +++ /var/folders/h1/2b6gts3x37zcnj5khyv8y5180000gn/T/tmp.Gz9LFK18/compile-no_vars_success.stderr.txt	2023-08-28 15:16:09
# @@ -0,0 +1 @@
# +----->  is not valid, please specify an exact Python version (e.g. 3.8.1 or ==3.8.1) in your pyproject.toml (and thus poetry.lock)
...

[zyv]

I'm running macOS 13.4.1 and I can't seem to get Python 3.8.1 installed, which seems to be required for the tests

Ah, I see - that explains it. No, Python 3.8.1 should absolutely not be required. In fact, I don't think that any version of Python is required to run the tests.

Apparently the tests are broken on macOS, because nobody ever tried to run them there and my shot in the dark seems to not have helped. I'll try to run them on my Mac and see how bad the problem appears to be. The core harness seems to be working, which is already surprising.

In general, the buildpack is written in shell, and it's notoriously hard to write really portable POSIX shell code depending only on the interpreter and core utilities. Which is why I was quite happy to get it to work on Linux in the first place and never contemplated porting to macOS, since nobody is using macOS in production in the first place, and lots of nasty surprises will have to be dealt with when considering the BSD userland.

But I've already wrote a portable POSIX test harness rendering JUnit reports, so how hard it can be... :-) the teenager zeal has definitively worn off by now after that experience though. Let's see...

[zyv]

I have fixed unportable sed usages and added CI to run tests on macOS.

[jacebrowning]

@zyv thanks for making the sed calls more portable!

I have added a test that passes for me locally:

bash run_tests.sh
1..19
ok - compile sanity-1
ok - compile sanity-2
ok - compile sanity-2
ok - compile no_vars_success
ok - compile poetry_version_comment
ok - compile exact_version_specifier
ok - compile trailing_space
ok - compile export_dev
ok - compile force_python_version
ok - compile invalid_python_version
ok - compile skip_runtime_success
ok - compile force_poetry_version
ok - compile skip_runtime_error
ok - compile export_params-0
ok - compile export_params-1
ok - detect sanity
ok - detect not_poetry
ok - detect poetry
ok - release sanity

But it fails on my fork's CI: https://github.com/jacebrowning/python-poetry-buildpack/actions/runs/6018844169/job/16327748151

Any idea why?

[zyv]

@jacebrowning , well your grep call was unportable - it works on macOS, but it wouldn't work on Linux and by extension on Heroku. Maybe you can see now, why we can't possibly accept anything without tests...

Could you please update the README? I think we are almost done here, or is there anything else left to do?

[jacebrowning]

@zyv I'll update the README now. That should be the last piece to mark this ready for review.

[zyv]

@marns93 @felix11h I've cleaned it up now and in as far as I'm concerned, it's good to go.

I think we should consider which way we want to take internally - set a specific version, our rely on the lock detection, but for most users I would assume the expected behaviour would be to use the version from the lock if it's there at all.

We could also consider getting rid of the hardcoded version completely now and just use the latest, if it's neither specified explicitly nor can be inferred from the lock, what do you think?

[marns93]

@marns93 @felix11h I've cleaned it up now and in as far as I'm concerned, it's good to go.

I think we should consider which way we want to take internally - set a specific version, our rely on the lock detection, but for most users I would assume the expected behaviour would be to use the version from the lock if it's there at all.

I think we should use it from poetry.lock as well.

We could also consider getting rid of the hardcoded version completely now and just use the latest, if it's neither specified explicitly nor can be inferred from the lock, what do you think?

This is a good idea, but I need to think about the consequences. Let's finish this PR as it is and think about this for the future.

[marns93]

👍 Thanks for participating.

[zyv]

👍 Thanks for participating.

So what is your opinion on removing the hardcoded version altogether and falling back to latest? If you agree, then we can do it in this PR.

[jacebrowning]

So what is your opinion on removing the hardcoded version altogether and falling back to latest?

As a user of this buildpack, I would not find that behavior unexpected. For example, I believe heroku/python defaults to the latest supported Python when runtime.txt is absent.

But we could also merge this as-is to see what it's like to use in real projects. 😄

[marns93]

👍 Thanks for participating.

So what is your opinion on removing the hardcoded version altogether and falling back to latest? If you agree, then we can do it in this PR.

I agree, we can fall back to latest Poetry version. I think most of the users are using a version where the Poetry version is in the poetry.lock already.

[felix11h]

Thank you for preparing this! I'm also happy with using "latest" instead of the hard-coded version.

[zyv]

Thank you for preparing this! I'm also happy with using "latest" instead of the hard-coded version.

Alright, I have added another commit that should implement this.

[zyv]

@jacebrowning thank you for your contribution!