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

Fix file+noindex URI usage on Windows #10728

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Jan 8, 2025

This PR fixes the parsing of URIs for file+noindex repositories when using Windows paths. As suggested by @phadej in #10703 we now use (and specify in the docs) //./C:/... paths on Windows.

QA

In Windows, one can now specify //./ paths in file+noindex repositories. To check, create a simple package, then cabal sdist, move the tar.gz to some directory and in a different project declare the following stanza:

repository my-local-repo
  url: file+noindex:////./C:/path/to/repo

It might still fail because of #9891


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!) fixing other tests failures seems like a good enough test for me

@jasagredo jasagredo force-pushed the js/local-noindex branch 3 times, most recently from 8cacda3 to fe6eed7 Compare January 9, 2025 00:44
@jasagredo jasagredo changed the title WIP fix local+noindex on Windows Fix file+noindex URI usage on Windows Jan 9, 2025
@jasagredo jasagredo marked this pull request as ready for review January 9, 2025 00:46
reponame
-- Normalization of Windows paths that use @//./@ does not fully
-- normalize the path (see filepath#247), but it is still usable.
(normalise (uriPath uri))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The key point of this PR is this call to normalise.

ghci> readFile "//./C:/some/path"
*** Exception: //./C:/some/path: openFile: does not exist (No such file or directory)
ghci> readFile $ normalise "//./C:/some/path"
"The contents of the file in C:/some/path\n"

@jasagredo
Copy link
Collaborator Author

Notice this was broken on Windows #10095 (comment), see how the paths in the output do not have a C: component, because of what I described in the first code block of #10703

Cabal-syntax/src/Distribution/Utils/Path.hs Outdated Show resolved Hide resolved
Cabal-syntax/src/Distribution/Utils/Path.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks! I wonder if we can pull in some of Andrea’s PRs after this fix…

@@ -200,6 +200,10 @@ repository.
``package-name-version.tar.gz`` files in the directory, and will use optional
corresponding ``package-name-version.cabal`` files as new revisions.

.. note::
On Windows systems, the path has to be prefixed by ``//./`` as in
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changelog says “can”, but docs says “has to”. These are different things, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant to say that now they can use local+noindex repos, previously they couldn't. And if they want to use it they have to use that syntax.

Isn't the wording right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the "if" part that you say here (if they want to use it they have to use that syntax) is not in the docs. The docs just say "has". Could you add the "if" part?

changelog.d/pr-10728 Show resolved Hide resolved
-- normalized) as a POSIX path, for example in URIs.
asPosixPath :: FilePath -> FilePath
asPosixPath p =
[if x == Windows.pathSeparator then Posix.pathSeparator else x | x <- p]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs don't recommend direct comparison but to use isPathSeparator instead.

Rather than using (== pathSeparator), use this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im fine using isPathSeparator but keep in mind that

> Windows.isPathSeparator Posix.pathSeparator
True

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw that too. I'm OK either way. Maybe a short comment attached to your choice?

Comment on lines +546 to +547
-- | Sometimes we need to represent a Windows path (that might have been
-- normalized) as a POSIX path, for example in URIs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any added tests with this pull request. Would it be possible to add some? How about a doctest example attached to the haddocks of asPosixPath?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do that for asPosixPath. The tests in PackageTests now pass without the hack in the output normalizer, I consider that sufficient enough proof for this small change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you point me to a failed CI run or test before this fix?

Copy link
Collaborator Author

@jasagredo jasagredo Jan 9, 2025

Choose a reason for hiding this comment

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

Would this be what you are looking for? https://github.com/haskell/cabal/actions/runs/12402807907/job/34625090748?pr=10647 It stopped failing when a hack was put in place, the lines I remove in the OutputNormalizer

Also you might want to look at this comment and above in the Cabal matrix channel https://matrix.to/#/!YWYQDCuIpuQHzGrLzd:matrix.org/$TFlqqD7_hY3361aMFALPvP4Yzr3Ib7zMTNlu-_x0q-I?via=matrix.org&via=kf8nh.com&via=tchncs.de

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(On the other hand, asPosixPath is so trivial that I don't see a huge benefit adding doctests, but if you insist I will do it)


- Windows users can now specify file+noindex paths using the format `file+noindex:////./C:/path/to/repo`.
This syntax comes from https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#dos-device-paths,
and is the only syntax for DOS paths fully supported by the `network-uri` package.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
and is the only syntax for DOS paths fully supported by the `network-uri` package.
and is the only syntax for DOS paths fully supported by the `network-uri` package,
which Cabal uses to handle paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file+noindex parsing is broken for Windows paths
3 participants