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

supply: remove pre-installing mercurial #576

Merged
merged 1 commit into from
Aug 30, 2022
Merged

supply: remove pre-installing mercurial #576

merged 1 commit into from
Aug 30, 2022

Conversation

arjun024
Copy link
Member

@arjun024 arjun024 commented Aug 24, 2022

Edited Fri 8/26 with more details

What does this change address?

  • There's no documentation that suggests that while installing from
    a remote url via a VCS like hg, the mercurial package should be
    pre-installed. Pip documentation just states that it requires a working
    executable to be available, which already exists on the stack.
‣ docker run --init -it cloudfoundry/cflinuxfs3 bash -c "hg --version"
Mercurial Distributed SCM (version 4.5.3)
  • When fixtures/mercurial is built by this branch's buildpack,
    we can see in the log python-hglib (which was the package referred to in
    the testdata by hg clone url) is installed.
Successfully installed Flask-2.2.2 ... python-hglib-2.6.2+2.1e7a64588ab0 ...

What does this change NOT address?

  • This change does not address why running an app with mercurial present in
    the requirements.txt fails with the error pointing to a non-existent include path to
    Python.h even after include location is set via CFLAGS in 028a7b6.
    See CI log. The timing of this failure appearing in CI suggests that it's related to the
    package using the new setuptools version as a transitive dependency. See python3.8.12 - fatal error: Python.h: No such file or directory #574
    This has to be separately investigated.

@arjun024
Copy link
Member Author

arjun024 commented Aug 24, 2022

Regarding the What does this change NOT address? section:
Some things I tried - uncommented and put into one diff: ed26b63

@arjun024 arjun024 marked this pull request as ready for review August 26, 2022 19:37
* There's no documentation that suggests that while installing from
a remote url via a VCS like `hg`, the `mercurial` package should be
pre-installed. [Pip documentation](https://pip.pypa.io/en/latest/topics/vcs-support/) just states that it requires a working
executable to be available, which already exists on the stack.
```
‣ docker run --init -it cloudfoundry/cflinuxfs3 bash -c "hg --version"
Mercurial Distributed SCM (version 4.5.3)
```

* When `fixtures/mercurial` is built by this branch's buildpack,
we can see in the log python-hglib (which was the package referred to in
the testdata by hg clone url) is installed.
```
Successfully installed Flask-2.2.2 ... python-hglib-2.6.2+2.1e7a64588ab0 ...
```

* Git history suggests that pre-installing mercurial via `pip install
mercurial`
([link](https://github.com/cloudfoundry/python-buildpack/blob/v1.7.58/src/python/supply/supply.go#L201-L215))
came into this buildpack from the original heroku buildpack fork.
Heroku has since removed it. See
heroku/heroku-buildpack-python#1111

* This change does not address why running an app with `mercurial`
present in the `requirements.txt` fails with the error pointing to a
non-existent include path to `Python.h` even after include location is
set via CFLAGS in 028a7b6.  See [CI
log](https://buildpacks.ci.cf-app.com/teams/main/pipelines/python-buildpack/jobs/specs-edge-integration-develop/builds/1054#L62d6d57b:516).
The timing of this failure appearing in CI suggests that it's related to
the package using the new setuptools version as a transitive dependency.
See #574 This has to be separately investigated.
@arjun024 arjun024 merged commit e9b6ee3 into develop Aug 30, 2022
@arjun024 arjun024 deleted the mercurial branch August 30, 2022 19:17
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.

4 participants