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

Path based dev-dependencies break 'install --no-dev' when the dir does not exist #210

Closed

Conversation

ride90
Copy link

@ride90 ride90 commented Oct 1, 2021

Resolves: python-poetry/poetry#668

  • Added tests for changed code.
  • Updated documentation for changed code.

@ride90 ride90 changed the title Path based dev-dependencies break 'install --no-dev' when the dir does not exist WIP Path based dev-dependencies break 'install --no-dev' when the dir does not exist Oct 1, 2021
@ride90
Copy link
Author

ride90 commented Oct 1, 2021

Hello @sdispater , please take a look and let me know if it makes sense, if so I will provide tests and update docs.
Currently marking PR as WIP.
🆙

@ride90
Copy link
Author

ride90 commented Oct 12, 2021

Hey, please, any feedback? 🙏

@ride90
Copy link
Author

ride90 commented Oct 20, 2021

Hi @sdispater , is there any chance to get a feedback?
Thanks.

@ride90
Copy link
Author

ride90 commented Nov 8, 2021

Hi @stephsamson , is there any chance to get a feedback from any of the maintainers?
Cheers!

@ride90
Copy link
Author

ride90 commented Dec 8, 2021

Hey, is it possible to get a feedback?

@ride90 ride90 changed the title WIP Path based dev-dependencies break 'install --no-dev' when the dir does not exist Path based dev-dependencies break 'install --no-dev' when the dir does not exist Dec 8, 2021
@haf
Copy link

haf commented Dec 18, 2021

This would be great to merge. Installing all deps takes between 5 and 10 minutes because changes to the source code causes cache busting for docker containers. This leads to every commit consuming N x 10 minutes longer of build time, where N=3 in our small case.

@ride90
Copy link
Author

ride90 commented Jan 12, 2022

Hi @sdispater , please can you comment on this? 🙏
It would be great to have at least some feedback.

@marcelmindemann
Copy link

Please @sdispater @abn @dimbleby @radoering @finswimmer @branchvincent,
show some love to this good man and review his PR ❤️
This bug is currently a show stopper for using poetry when building docker images. Path dependencies should be installed differently from regular dependencies in Dockerfiles, and sorting path dependencies under dev dependencies is the easiest way to differentiate.

@radoering
Copy link
Member

radoering commented Jun 17, 2022

Sorry for the late feedback. I took a quick look and I have some concerns:

  • Using sys.args at this place doesn't seem right.
  • There are not only two dependency groups anymore. --no-dev will be deprecated in 1.2.
  • The warning instead of an error is only fine if a lock file exists. If there is no lock file, dev dependencies are required for implicit locking before install.

Unfortunately, some logic to determine if a warning is sufficient seems like a brittle fix that will easily break to me.

Nevertheless, the __init__ of DirectoryDependency (and FileDependency) might be too early for checking if the path exists. (URLDependency and VCSDependency do not check if the package is available in __init__). Maybe, there should just be some lazy evaluation: check for existence only if full_path is used for the first time?

@mbelang
Copy link

mbelang commented Aug 5, 2022

Any progress here?

@radoering
Copy link
Member

Since nothing has happened here for quite some time to address the concerns (which might be due to the late feedback), I'm closing this PR in favor of #520, which takes a more general approach. Nevertheless, thanks for your contribution.

@radoering radoering closed this Nov 13, 2022
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.

Do not validate path dependencies when they are not selected for installation
5 participants