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

Should by default add build-system.requires entries as dev-dependencies #3428

Closed
2 tasks done
filmor opened this issue Nov 29, 2020 · 22 comments
Closed
2 tasks done

Should by default add build-system.requires entries as dev-dependencies #3428

filmor opened this issue Nov 29, 2020 · 22 comments
Labels
kind/feature Feature requests/implementations

Comments

@filmor
Copy link

filmor commented Nov 29, 2020

  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have searched the documentation and believe that my question is not covered.

Feature Request

Everything that is listed in build-system.requires is probably a dev-dependency and it would be good, if it was added automatically s.t. one does not have to keep both lists in sync. I can currently not thing of any situation where this would not be beneficial :)

@filmor filmor added kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels Nov 29, 2020
@n8sty
Copy link
Contributor

n8sty commented Nov 30, 2020

To build a library it's not necessary to actually have any of the development dependencies installed. For example, the default that requires Poetry as the build system (what's generated by poetry new) can build a wheel without installing anything (except for Poetry).

@filmor
Copy link
Author

filmor commented Nov 30, 2020

I'm aware of that. What I wrote is exactly the other way round.

/edit: To elaborate a bit, I have an additional build (not runtime) dependency to poetry-core that I list in the requires entry. If I do a straight poetry build, it's not picked up and the build fails.

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

@filmor I guess I see what you mean, and there is a good point in there. This could probably be an opt-in setting (deactivated by default).


I have an additional build (not runtime) dependency to poetry-core that I list in the requires entry. If I do a straight poetry build, it's not picked up and the build fails.

This does not seem normal at all. Or maybe I am misunderstanding something. Would you mind expanding on this?

@filmor
Copy link
Author

filmor commented Nov 30, 2020

In this particular case it is about https://github.com/pythonnet/pythonnet which requires pycparser at build time but not at runtime. Distinguishing build and runtime dependencies is not so uncommon, just look at the split between poetry and poetry-core.

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

I am confused...

In this particular case it is about https://github.com/pythonnet/pythonnet which requires pycparser at build time but not at runtime.

The example you linked, shows the exact opposite, pycparser is set as a run-time dependency since it is added to the install_requires list of setuptools.setup.

Distinguishing build and runtime dependencies is not so uncommon, just look at the split between poetry and poetry-core.

Yes, and poetry (via PEP517/PEP518) allows to do that perfectly well. Right?

@filmor
Copy link
Author

filmor commented Nov 30, 2020

I'm working on improving the build of pythonnet, and indeed, up until now pycparser was just pulled in as a runtime dependency, but it's not required at runtime.

To get back to topic:

A rough pyproject.toml for pythonnet may look like

[build-system]
requires = ["poetry_core>=1.0.0", "pycparser"]
build-backend = "poetry.core.masonry.api"

# ...

[tool.poetry.build]
generate-setup-file = false
script = "tools/build.py"

[tool.poetry.dependencies]
python = ">=3.6 <3.10"

[tool.poetry.dev-dependencies]
pytest = "~6"
psutil = "~5"
pycparser = "~2"

The current state is at https://github.com/filmor/pythonnet/blob/master/pyproject.toml. I don't like that I have to list pycparser both in build-system.requires as well as in tool.poetry.dev-dependencies. I can't think of a case where I'd need something for building but not during development using just poetry.

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

I'm working on improving the build of pythonnet, and indeed, up until now pycparser was just pulled in as a runtime dependency, but it's not required at runtime.

Ah, OK, thanks for clarification.

I don't like that I have to list pycparser both in build-system.requires as well as in tool.poetry.dev-dependencies.

Yes, I agree that duplications should be avoided as much as possible.

I can't think of a case where I'd need something for building but not during development using just poetry.

Well, I'd argue that seeing as you seem to be the first one to ask for such a feature (to my knowledge), it might be an indication that having the build dependencies installed in the development environment is not a particularly needed feature... Thinking about it, I'd venture that it is most likely a consequence of the fact that poetry does not (officially) support custom build steps. If there are no custom build steps then having anything else than poetry-core in the build dependencies seems quite unlikely. And I'd also think that there are not many reasons to have poetry-core installed in the development environment either.

@filmor
Copy link
Author

filmor commented Nov 30, 2020

poetry-core will always be installed if you are using poetry, as it's a dependency of poetry itself. It's actually really simple to add this behaviour and it would be a good preparation for further improvements of custom build steps. I'll create a PR and we can continue the discussion over there.

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

poetry-core will always be installed if you are using poetry, as it's a dependency of poetry itself.

What? No. Neither poetry nor poetry-core are installed in the development virtual environment. And they shouldn't. Why would someone want to have poetry or poetry-core in their development environment? I am very confused... Are you maybe talking about the build environment?

It's actually really simple to add this behaviour and it would be a good preparation for further improvements of custom build steps. I'll create a PR and we can continue the discussion over there.

I am really not following in which direction you are going. OK, let's wait for your PR...

@filmor
Copy link
Author

filmor commented Nov 30, 2020

Hmm, maybe I'm indeed confusing build and dev here, I wasn't aware of a distinction. Is my concrete problem clear? I essentially want to have everything that is needed to run my build-script (I extended the post above with the respective section) available after I run poetry install. This will currently require me to note the additional build dependency (pycparser in this case) in both the build-system.requires as well as in the tool.poetry.dev-dependencies sections in the pyproject.toml.

My idea to resolve this was to just always amend the former to the latter.

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

Hmm, maybe I'm indeed confusing build and dev here, I wasn't aware of a distinction.

OK, that might explain our disagreements.

In short: since PEP517 and PEP 518, build front-ends (such as poetry, pip, etc.) tend to use a technique called build isolation in order to build (i.e. transform a sdist into a wheel for example) Python projects (i.e. pip-installable libraries found on PyPI for example). Basically an ephemeral virtual environment is created, then the build dependencies listed under requires in the [build-system] section of pyproject.toml are installed in that environment, and finally the function specified under build-backend of that same section is called. The resulting distribution archive file (typically a sdist or a wheel) is saved.

So dev-dependencies are a completely different list of dependencies, and those lists do not necessarily intersect. If poetry were to support custom build dependencies, then there might be a good reason to install the build dependencies in the development virtual environment as well. A bit far fetched, I am still not sure it would be necessary, but maybe it could be helpful. But poetry does not (officially) support custom build dependencies anyway, so there is not much need to begin with.

Is my concrete problem clear? I essentially want to have everything that is needed to run my build-script (I extended the post above with the respective section) available after I run poetry install.

Somewhat clear, yes. But I feel like maybe your intentions here are based on misconceptions of the current situation. Are you really sure you need your build dependencies in the development environment? What would you use them for? Isn't it enough to call poetry install and/or poetry build? Those two commands should take care of installing the build dependencies (in an isolated build environment) if they are needed.

This will currently require me to note the additional build dependency (pycparser in this case) in both the build-system.requires as well as in the tool.poetry.dev-dependencies sections in the pyproject.toml.

My idea to resolve this was to just always amend the former to the latter.

Sure, duplications should be avoided. But I am still not sold on the premise.

We'll see what the maintainers say...

@filmor
Copy link
Author

filmor commented Nov 30, 2020

Somewhat clear, yes. But I feel like maybe your intentions here are based on misconceptions of the current situation. Are you really sure you need your build dependencies in the development environment? What would you use them for? Isn't it enough to call poetry install and/or poetry build? Those two commands should take care of installing the build dependencies (in an isolated build environment) if they are needed.

I've tried that and they don't. Only pip install . is successful if I only list the build dependencies in build-system.requires. That's exactly why I opened this ticket :-)

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

I've tried that and they don't.

What have you tried?

Only pip install . is successful if I only list the build dependencies in build-system.requires.

What has failed? What were the error messages?

@filmor
Copy link
Author

filmor commented Nov 30, 2020

It doesn't find pycparser during the build script execution: https://pipelines.actions.githubusercontent.com/gouKMoZ3pz4f0c6qjPt6CztOzBAt3KwfQCImeMc1nkzTD8UN5i/_apis/pipelines/1/runs/20/signedlogcontent/3?urlExpires=2020-11-30T16%3A36%3A56.2770160Z&urlSigningMethod=HMACV1&urlSignature=749fjmjbYzF%2Fk7w25RzOokY7wktyT39%2FQeFY%2BzD7kEM%3D

What I did was simply checking out the repo and doing poetry install in the directory. I'll create a minimal example with something less error-prone (pycparser is actually a transitive dependency of poetry through something that uses cffi).

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

I'll create a minimal example with something less error-prone

That would be great!

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

@filmor
Copy link
Author

filmor commented Nov 30, 2020

This one should make the problem clear: https://github.com/filmor/issue3428

For me, pip install . works completely fine while poetry install fails because print_color is not installed. I'll test whether it works with my proposed patch.

@sinoroc
Copy link

sinoroc commented Nov 30, 2020

This one should make the problem clear: https://github.com/filmor/issue3428

404 for me. Maybe the repo is not public.

@filmor
Copy link
Author

filmor commented Dec 1, 2020

Indeed, github-cli does that by default, apparently :)

Also, I'm apparently not the first one stumbling over this: #2789

@abn's solution used "build" as the category, which I think is the better solution.

@sinoroc
Copy link

sinoroc commented Dec 1, 2020

I can recreate the issue. I am really surprised poetry install fails on this. I assumed the build happens in isolation in such a case as well, but obviously it doesn't. I'd argue it should happen in isolation. Anyway now it is clear that the maintainers are aware of the situation. Should we close this ticket in favour of #2789?

@filmor
Copy link
Author

filmor commented Dec 9, 2020

That's probably fine.

@filmor filmor closed this as completed Dec 9, 2020
@abn abn removed the status/triage This issue needs to be triaged label Mar 3, 2022
Copy link

github-actions bot commented Mar 2, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants