poetry-core icon indicating copy to clipboard operation
poetry-core copied to clipboard

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

Open ride90 opened this issue 3 years ago • 10 comments

Resolves: python-poetry/poetry#668

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

ride90 avatar Oct 01 '21 19:10 ride90

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 avatar Oct 01 '21 19:10 ride90

Hey, please, any feedback? 🙏

ride90 avatar Oct 12 '21 10:10 ride90

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

ride90 avatar Oct 20 '21 07:10 ride90

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

ride90 avatar Nov 08 '21 08:11 ride90

Hey, is it possible to get a feedback?

ride90 avatar Dec 08 '21 14:12 ride90

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.

haf avatar Dec 18 '21 01:12 haf

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

ride90 avatar Jan 12 '22 16:01 ride90

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.

marcelmindemann avatar Jun 17 '22 10:06 marcelmindemann

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?

radoering avatar Jun 17 '22 15:06 radoering

Any progress here?

mbelang avatar Aug 05 '22 20:08 mbelang

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 avatar Nov 13 '22 11:11 radoering