search

mesonbuild / meson-python

Meson PEP 517 Python build backend
https://mesonbuild.com/meson-python/
MIT License
118 stars 59 forks source link

ENH: do not include uncommitted changes in the sdist #587

Closed dnicolodi closed 1 month ago

dnicolodi commented 4 months ago

Including uncommitted changes in the sdist was implemented in #58 after the discussion in #53. However, the current behavior for which uncommitted changes to files under version control are included but not other files, is an hard to justify surprising half measure.

After careful analysis, it has been determined that none of the use cases for this feature is still valid. Removing it makes the implementation easier, and the behavior less surprising and easier to document an explain.

rgommers commented 4 months ago

Thanks. Quick comment: could you update the sdist docs in docs/explanations/design-old.rst for this change, and mention add_dist_script there?

dnicolodi commented 4 months ago

design-old.rst is not linked anywhere, thus it is not published. I thought we keep it only as base for some future documentation work. This is why I haven't updated it. I'm also working on some documentation updates, but the job got bigger than anticipated and it is not finished yet.

rgommers commented 4 months ago

design-old.rst is not linked anywhere, thus it is not published. I thought we keep it only as base for some future documentation work

You're right. My bad, that's what I get for trying to squeeze in a quick comment before signing off. I saw the removal of the add_dist_script comment, then looked for a mention in the docs, didn't find it, then looked for .gitattributes. Our only mention of that is in design-old.rst. Looks like we should revive some of that content and link to https://mesonbuild.com/Creating-releases.html

dnicolodi commented 4 months ago

Should we merge this and leave the documentation updates to a separate PR?

rgommers commented 3 months ago

I'm okay with doing the docs in a follow-up. This is ready to go, right? If so, I need to do some testing with numpy, scipy, & co.

dnicolodi commented 3 months ago

Yes, it is ready to go. The documentation updates are coming soon.

QuLogic commented 3 months ago

Some typos: s/changer/changes/ in the PR title and commit summary, and s/an hard/a hard/ and s/document an explain/document and explain/ in the description.

rgommers commented 2 months ago

@dnicolodi I'd like to merge this for the next release after doing some final testing (likely tomorrow). Would you mind resolving the conflict and fixing the commit message typos pointed out by @QuLogic?

dnicolodi commented 2 months ago

@rgommers Sure, done.

dnicolodi commented 2 months ago

@rgommers I fixed the ownership metadata in the sdist archive to be root:root (not only to use the 0 uid and gid as it was implicitly done before). This follows what git archive does. I left the permissions unchanged: we use the same permissions exported by git archive. It seems a valid choice to me. I added another commit that makes the sdist byte-for-byte reproducible in most cases, see the longish comments in the code. I think it is a worthwhile goal and it relatively easy to achieve. Finally, I promoted and expanded the little documentation about sdist we had in the unpublished document to its own section in the manual. Please have a look.

rgommers commented 2 months ago

That sounds great! Thanks for improving the docs too. I'll try and revisit this in the morning.

dnicolodi commented 2 months ago

I'm very surprised that this happens, especially by the path that two different paths are used. I don't see how that can happen. Let me see if I can reproduce it.

dnicolodi commented 2 months ago

Actually, we have tests where the project name passed to Meson's project() and the Python distribution name differ, and these work just fine. I really don't see how what you see could happen. Also, the code that adjusts the paths didn't change compared to what you tested previously. Are you sure that the test is correct?

dnicolodi commented 2 months ago

I can reproduce it building an sdist for numpy. But I really cannot get how it can happen.

dnicolodi commented 2 months ago

All the paths that are not correctly rewritten are longer than 100 characters. It looks like a Python bug to me. Investigating further.

dnicolodi commented 2 months ago

Yup. I'm pretty sure it is a Python tarfile module bug: when the name filed in the TarInfo object is set, and the object has an associated extended pax header, the path property in the pax extended header is not reset. If the new name is shorter than what requires the path to be stored in the extended pax header, the path in the pax header is not updated when the archive member is serialized. When the header is read back, the path in the extended path header is read, instead of the shorter one in the regular header. The work around is simple. I'll implement it with a test tomorrow.

dnicolodi commented 2 months ago

Issue fixed and test added. This was a funny one to track down.

dnicolodi commented 2 months ago

The codecov CI job is stuck and I didn't find a way to kill it.

rgommers commented 2 months ago

@dnicolodi it looks like you didn't actually push your fix?

dnicolodi commented 2 months ago

Indeed. Pushed now. I think I was waiting to file a bug for CPython to be able to reference it in the code comment and commit message, but I haven't submitted it yet. I don't think it is important to reference it here, and I can add a reference later. Pushed now.