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

Automatically include license files in .dist-info/license_files #862

Merged
merged 10 commits into from Apr 2, 2022

Conversation

idavis
Copy link
Contributor

@idavis idavis commented Apr 1, 2022

Implementing parts of #861 for PEP 639 not requiring updates to the metadata version. The changes in #837 added the .dist-info/license_files using the License-File field.

Any files in the project root matching glob patterns ["LICEN[CS]E*", "COPYING*", "NOTICE*", "AUTHORS*"] (from PEP 639) are automatically added to the license files list.

Further work for PEP 639 requires update to the metadata versions and approval of 639. The serialization of the fields has been handled by Adding license_expression and license_files serialization. Additionally, the wheel writer trims paths to write only the raw file name into the .dist-info/license_files. Future work will need to modify this behavior as PEP 639 requires relative tree paths to be reflected in the .dist-info/license_files hierarchy.

@netlify
Copy link

netlify bot commented Apr 1, 2022

Deploy Preview for maturin-guide canceled.

Name Link
🔨 Latest commit 0c1a530
🔍 Latest deploy log https://app.netlify.com/sites/maturin-guide/deploys/62484ddbf4fdc50008473941

@idavis idavis force-pushed the default-dynamic-license-files branch from da508c4 to 11dde2e Compare April 1, 2022 17:14
src/metadata.rs Outdated
let license_files_strings: Vec<_> = metadata
.license_files
.iter()
.map(|v| v.to_str().unwrap())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can just keep everything as PathBuf and drop the unwrap

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed fix, thank you!

src/metadata.rs Outdated
let license_include_targets = ["LICEN[CS]E*", "COPYING*", "NOTICE*", "AUTHORS*"];
for pattern in license_include_targets.iter() {
for license_path in glob::glob(&manifest_path.join(pattern).to_string_lossy())
.expect("No license files found for pattern")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the manifest_path needs to be escaped with glob::escape or it'll become part of the pattern. The expect shouldn't happen anyway, but a? would be better than possibly panicking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a fix for the ? usage instead of the expect. I followed the pattern from source_distribution.rs#L317-L328. I can update the code to used Pattern::escape if you like.

    if let Some(include_targets) = sdist_include {
        for pattern in include_targets {
            println!("📦 Including files matching \"{}\"", pattern);
            for source in glob::glob(&manifest_dir.join(pattern).to_string_lossy())
                .expect("No files found for pattern")
                .filter_map(Result::ok)
            {
                let target = root_dir.join(&source.strip_prefix(manifest_dir)?);
                writer.add_file(target, source)?;
            }
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konstin Is this what you are looking for?

let license_include_targets = ["LICEN[CS]E*", "COPYING*", "NOTICE*", "AUTHORS*"];
let escaped_manifest_string = glob::Pattern::escape(manifest_path.to_str().unwrap());
let escaped_manifest_path = Path::new(&escaped_manifest_string);
for pattern in license_include_targets.iter() {
    for license_path in
        glob::glob(&escaped_manifest_path.join(pattern).to_string_lossy())?
            .filter_map(Result::ok)
    {
//....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that looks good!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konstin Just pushed the change. Thanks for the tip.

Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@messense messense merged commit 2615131 into PyO3:main Apr 2, 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.

None yet

3 participants