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

Refactor path handling. Prefer absolute paths wherever possible. #35

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

dfederm
Copy link
Member

@dfederm dfederm commented Jan 18, 2024

Today paths are handled in a haphazard way, somewhat as a holdover from QuickBuild combined with the logic used to make paths portable.

Previously a "normalized" path was either repo-relative, or had placeholders, depending on context. Also, most paths were repo-relative, which is causing problems now that we're trying to do more with other paths outside the repo, like paths in the NuGet package cache.

This change rationalizes things by defining the following wrt paths:

  • "Absolute" paths are full rooted paths. This form is preferred in most cases.
  • "Relative" paths are relative to the repo root. This form is mostly used with outputs since we (probably) will never consider outputs outside the repo, and also where it makes sense like for logging a project file path.
  • "Normalized" paths are absolute paths with well-known base paths replaced with placeholders, eg the repo root or the nuget package cache. This form is used in logging to make log files more easily diffable, as well as in cached metadata to make paths portable across machines.

Additionally, BXL's AbsolutePath adds a long path prefix (\\?\) to long paths automatically, which messes with some of our string manipulation. For ease of use, I've avoided all usage of AbsolutePath except just before talking to the BXL APIs which require them. The alternative would be to use AbsolutePath literally everywhere and add some helpers to their code for things like making relative paths, but I feel like tying so strongly to BXL's concept of paths isn't a great idea, especially since this project generically (barring specific impls) should not really be strongly tied to BXL's cache code at all. It is today unfortunately due to the local cache, but one day I hope to break that.

@dfederm dfederm force-pushed the normalization-refactor branch 3 times, most recently from 108ac3f to 681bc7c Compare January 18, 2024 22:23
@dfederm dfederm enabled auto-merge (squash) January 18, 2024 22:24
@dfederm dfederm mentioned this pull request Jan 19, 2024
@dfederm dfederm force-pushed the normalization-refactor branch from 2775553 to 32fe4b6 Compare January 20, 2024 00:51
@dfederm
Copy link
Member Author

dfederm commented Jan 22, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dfederm dfederm merged commit 2f29125 into main Jan 22, 2024
6 checks passed
@dfederm dfederm deleted the normalization-refactor branch January 22, 2024 22:01
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.

2 participants