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

Fix multiple packages build #292

Merged

Conversation

lucasiscovici2
Copy link
Contributor

@lucasiscovici2 lucasiscovici2 commented Feb 12, 2022

Resolves: multiple packages build

When there are multiple packages with the "from" keyword, the poetry build command, doesn't install the packages as expected.
Ex:

packages = [
    { include = "XXX", from = "path/to/dir1" },
    { include = "YYY", from = "path/to/dir2" },
    { include = "ZZZ", from = "path/to/dir3" },
    { include = "A" }
]

In fact, when poetry build, its find all the files and directories inside the path specified by the keyword 'from' ('find_files_to_add' function from poetry/core/masonry/builders/builder.py#L157) .
Next, if it's a directory, it find all files and add them with the 'BuildIncludeFile' class using the current path (self._path).
BuildIncludeFile with self._path
poetry/core/masonry/builders/builder.py#L176-L180

 include_file = BuildIncludeFile(
     path=current_file,
     project_root=self._path,
     source_root=self._path,
 )

If it's a file, it add it with the 'BuildIncludeFile' BUT using the 'specified from keyword' (include.base) if it is defined.
BuildIncludeFile with include.base
poetry/core/masonry/builders/builder.py#L188-L199

  if (
      isinstance(include, PackageInclude)
      and include.source
      and self.format == "wheel"
  ):
      source_root = include.base
  else:
      source_root = self._path


  include_file = BuildIncludeFile(
      path=file, project_root=self._path, source_root=source_root
  )

This Pr is intented to use the 'include.base' when it's the directory (if from is defined).
So only move up to the line #L172

  if (
      isinstance(include, PackageInclude)
      and include.source
      and self.format == "wheel"
  ):
      source_root = include.base
  else:
      source_root = self._path

And use source_root=source_root for the 'BuildIncludeFile' class in the directory part (#L179).

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

@lucasiscovici2 lucasiscovici2 changed the title create draft Fix multiple packages build Feb 12, 2022
@lucasiscovici2 lucasiscovici2 marked this pull request as ready for review February 12, 2022 02:05
@lucasiscovici2
Copy link
Contributor Author

@python-poetry/triage

@lucasiscovici2
Copy link
Contributor Author

lucasiscovici2 commented Feb 28, 2022

@abn

@finswimmer
Copy link
Member

Hey @lucasiscovici-Loreal,

could you please add test(s) that cover your changes?

Thanks a lot for your contribution 👍

fin swimmer

@kurtmckee
Copy link
Contributor

@lucasiscovici-Loreal, thanks for the mention. I'm not yet clear what I need to be aware of, or what help I can offer. How can I help here?

@lucasiscovici2
Copy link
Contributor Author

@lucasiscovici-Loreal, thanks for the mention. I'm not yet clear what I need to be aware of, or what help I can offer. How can I help here?

Hello @kurtmckee, my bad, I wanted to talk to the poetry maintainers

@lucasiscovici2
Copy link
Contributor Author

Hey @lucasiscovici-Loreal,

could you please add test(s) that cover your changes?

Thanks a lot for your contribution 👍

fin swimmer

Hello, @finswimmer, it's ok

@sonarcloud
Copy link

sonarcloud bot commented Mar 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
2.1% 2.1% Duplication

@abn abn merged commit 0c5cfa4 into python-poetry:master Mar 9, 2022
@lucasiscovici2
Copy link
Contributor Author

@abn @finswimmer hello when the new release please ?

@finswimmer finswimmer mentioned this pull request May 20, 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.

4 participants