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

Path handling needs overhaul #273

Closed
mbolivar opened this issue Jun 7, 2019 · 6 comments · Fixed by #411
Closed

Path handling needs overhaul #273

mbolivar opened this issue Jun 7, 2019 · 6 comments · Fixed by #411
Labels
enhancement New feature or request help wanted Implementation help is requested

Comments

@mbolivar
Copy link
Contributor

mbolivar commented Jun 7, 2019

The path handling in west was originally written and tested on Linux, and did things like using string comparison operators to test path equality after calling os.path.abspath or realpath.

After user testing on windows, we started throwing some normpath calls around to resolve issues related to case insensitivity and the POSIX vs. Windows path separators (i.e. slash vs. backslash).

The whole thing has evolved organically to keep things "working", but needs to be overhauled, with all path handling using the pathlib module and its platform-specific helpers.

This should result in more readable code, anyway, since pathlib has various conviences. E.g. calls to os.path.join(a,b) can be replaced with a / b.

@mbolivar
Copy link
Contributor Author

mbolivar commented Jun 7, 2019

cc @marc-hb

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 7, 2019

As can be seen in zephyrproject-rtos/zephyr#16537 and a number of links from there, I've been masochistically testing symbolic links in the Zephyr build. Among various Zephyr issues proving that: 1. I'm the first one attempting this; 2. git killed /mnt/ and network filesystems; I found this more specific west issue:
When ZEPHYR_BASE has symbolic links, then the output of west list is different depending on whether the current directory is inside the west installation versus not. That's because west has this useful and desired logic: "search [real] getcwd() first otherwise fall back on [symbolic] ZEPHYR_BASE". The fun effect on cmake's module_name = RELPATH(...) is left as an exercise for the reader[*]

After experimenting with some west changes I feel like fixing west issues with symbolic links would take neither a lot of west code changes, nor a lot of effort and could be performed gradually. However, I found a somewhat "code style" / naming convention issue. west list -h, git grep -i -C 3 -E 'rea?lpath' and other searches shows a widespread pattern of confusion between absolute paths and real paths, I mean stuff like this:

self.abspath = os.path.realpath(...

After this introduction that I didn't spend enough time making shorter, two questions for @mbolivar and others:

  1. Considering a grander move to pathlib is desired, there's not point even starting to fix the pattern of symbolic confusion in the current os.path code style. Correct?

  2. If/whenever this west migration to pathlib happens, it should initially be "bug-for-bug" compatible with all existing symbolic link issues. Because you should always be careful to change only one thing at a time and such refactoring should ideally have zero functional change, at least initially. Correct?

cc @SebastianBoe , @carlescufi, @ulfalizer

If 1. and 2 are both correct then I will stop discussing symbolic links here and forever hold my peace (= make noise about symbolic links elsewhere)

Worst kept secret PS: Windows supports forward slashes '/' everywhere except in horrible and obsolete .BAT files and CMD.EXE. I digress.

[*] There's a symbolic link bug hidden besides most RELPATH / os.path.relpath() invocations.

@mbolivar
Copy link
Contributor Author

mbolivar commented Jun 7, 2019

I've been masochistically testing

Indeed 😄.

Considering a grander move to pathlib is desired, there's not point even starting to fix the pattern of symbolic confusion in the current os.path code style. Correct?

If it's quick and easy and solves a real problem, I'm fine, but if (as I suspect) it's neither, then there's no point if it's all going to get rewritten.

I'm writing release notes for west 0.6.0, which will remove the bootstrapper but probably slip the other open issues, and want to tackle cleanups like this for 0.7.0 along with the multimanifest work.

If/whenever this west migration to pathlib happens, it should initially be "bug-for-bug" compatible with all existing symbolic link issues. Because you should always be careful to change only one thing at a time and such refactoring should ideally have zero functional change, at least initially. Correct?

If you can provide some test cases, I would be happy to make sure that they pass, as you've got the most experience here. But I can make no guarantees, unfortunately, since nobody but masochists seems to be using symlinks and zephyr 😂

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 7, 2019

This masochism is just one part of another plan: the ability to reproduce builds in environments as different and varied as possible, see zephyrproject-rtos/zephyr#14593.
I may park that particular bit though, it's far from the top priority and I already spent too much time on it. zephyrproject-rtos/zephyr#16537 is already trying to catch the worst and reduce the scope.

@mbolivar
Copy link
Contributor Author

mbolivar commented Jun 7, 2019

This masochism is just one part of another plan: the ability to reproduce builds in environments as different and varied as possible, see zephyrproject-rtos/zephyr#14593.

So symlinks are an enabler for reproducible builds? Is that what you mean? If so, that might bump the priority for me.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 7, 2019

No, it's not that simple. The term "reproducible builds" is a bit deceiving because too simplistic. Plus Windows deserves reproducible builds too :-)

Let's look at https://reproducible-builds.org/tools/

reprotest
reprotest builds the same source code in different environments and then checks the binaries produced by the builds to see if changing the environment, without changing the source code, changed the generated binaries.

You should wonder what "different environments" means there. Great question. The answer is: there's no single answer, just different lists of changes that may matter differently to different people/projects. Think of these environment changes as feature requests. There are environment changes that everyone wants like user ID, home directory or... build date! There are other environment changes many but not all people want like a different Python version or different operating system. There are changes that no one realistically wants like a different compiler[*]. And then there may be future Zephyr developers who want to reproduce builds on NFS /mnt/... unless NFS dies first.

Here's a list of "most wanted" environment changes: https://reproducible-builds.org/docs/
You can infer a similar list from the description field of zephyrproject-rtos/zephyr#14593

[*] except comparing the outputs of minor compiler revisions can be very interesting.

@mbolivar mbolivar added priority: low help wanted Implementation help is requested and removed priority: low labels Oct 11, 2019
@mbolivar mbolivar added the enhancement New feature or request label Jan 30, 2020
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Jul 1, 2020
We've now re-worked the west internals to use pathlib everywhere that
platform-specific path magic was previously relying on canon_path()
or other os.path-based methods to do comparisons or otherwise
canonicalize paths.

We can therefore remove canon_path() and consider the internals of
path handling hopefully satisfactorily reworked. Any further issues
with path handling should be treated as bugs.

Fixes: zephyrproject-rtos#273
Signed-off-by: Martí Bolívar <[email protected]>
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Jul 1, 2020
We've now re-worked the west internals to use pathlib everywhere that
platform-specific path magic was previously relying on canon_path()
or other os.path-based methods to do comparisons or otherwise
canonicalize paths.

We can therefore remove canon_path() and consider the internals of
path handling hopefully satisfactorily reworked. Any further issues
with path handling should be treated as bugs.

Fixes: zephyrproject-rtos#273
Signed-off-by: Martí Bolívar <[email protected]>
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Jul 8, 2020
We've now re-worked the west internals to use pathlib everywhere that
platform-specific path magic was previously relying on canon_path()
or other os.path-based methods to do comparisons or otherwise
canonicalize paths.

We can therefore remove canon_path() and consider the internals of
path handling hopefully satisfactorily reworked. Any further issues
with path handling should be treated as bugs.

Fixes: zephyrproject-rtos#273
Signed-off-by: Martí Bolívar <[email protected]>
mbolivar-nordic added a commit that referenced this issue Jul 8, 2020
We've now re-worked the west internals to use pathlib everywhere that
platform-specific path magic was previously relying on canon_path()
or other os.path-based methods to do comparisons or otherwise
canonicalize paths.

We can therefore remove canon_path() and consider the internals of
path handling hopefully satisfactorily reworked. Any further issues
with path handling should be treated as bugs.

Fixes: #273
Signed-off-by: Martí Bolívar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Implementation help is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants