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

scripts: Use pathlib to join destdir #10180

Closed
wants to merge 1 commit into from

Conversation

3v1n0
Copy link
Contributor

@3v1n0 3v1n0 commented Mar 24, 2022

Just a cleanup...

Coming from #10176

@eli-schwartz
Copy link
Member

Copying my thoughts over from the other PR...

The code comments say to consider whether pathlib can help here, but don't prescribe using it.

IMO it doesn't help us here. We're doing something not really natural to pathlib, and we have a battle-tested high level API on top of the low level string slicing.

Pathlib is a second level of abstractions, which ultimately does bit banging itself and then gives back information we have to do more bit banging on. The code isn't shorter and it's still ugly bit banging, so...

... We don't need to pay the cost of another layer of abstractions here in order to get no real cleanup, just shifting state.

@eli-schwartz
Copy link
Member

tl;dr the code comment was too optimistic.

@anadon
Copy link

anadon commented Apr 7, 2022

Coming in as someone more familiar with Python and less familiar with meson internals, I disagree with your assessment that there is no cleanup. To me, the code is more readable and approachable. It also communicates to me without deeper inspection that it is OS agnostic.

@QuLogic
Copy link
Member

QuLogic commented Apr 7, 2022

I disagree that the new code has bit banging; it's standard pathlib calls that happen to be doing an ugly thing because of how DESTDIR works.

Also, it would be even clearer if d1 -> destdir, d2 -> prefix. And it could also do os.path.join -> str(Path / Path).

return d1 + d2
p = pathlib.Path(d2)
if d1 and p.is_absolute():
p = p.relative_to(p.anchor)
Copy link
Member

Choose a reason for hiding this comment

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

If memory serves, relative_to throws an exception if the two paths are on different drives (e.g. c: and d:).

Copy link
Member

Choose a reason for hiding this comment

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

This is relative to its own anchor; it is not mixing paths, and cannot possibly trigger that.

@eli-schwartz
Copy link
Member

eli-schwartz commented Apr 7, 2022

Pathlib is not magic pixie dust that makes things clearer just because you use it. Pathlib is an implementation of an abstraction with the goal of providing low level utility functions for high level use.

This is a low level function doing something unnatural to pathlib. It's worked for a long time, like all the best abstractions. Everyone agrees it is ugly either way, and I personally think this is more ugly. I can't really tell what it does without a lot of reasoning about how pathlib works, and conversely, it takes me seconds to understand the method this switches away from.

I also find it very hard to tell without reading lots of pathlib documentation, whether it actually does the correct thing in all cases, and jpakkane suggests the answer is "no" in edge cases.

And most of all remember that this doesn't fix a bug or add better handling for any cases. It's just purely "the goal is to increase the use of pathlib".

Conversely, there are places we'd actually love to see pathlib used, because it will fix known bugs. backends.py and ninjabackend.py currently use lots of strings on regular non-DESTDIR paths and it's a source of actual bugs because we then do a find/replace on the entire command line and replace backslashes with forward slashes "to help with escaping on windows" that then break string literals which aren't paths at all.

That's an actual reason to switch to pathlib -- it's a case where pathlib is designed to help, and where it does help, and using it has a measurable improvement.

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.

5 participants