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

Merged
merged 1 commit into from
Jan 13, 2025

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
@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

@jasagredo jasagredo linked an issue Jan 9, 2025 that may be closed by this pull request
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…

doc/config.rst Show resolved Hide resolved
changelog.d/pr-10728 Outdated Show resolved Hide resolved
@jasagredo jasagredo force-pushed the js/local-noindex branch 3 times, most recently from 390ce77 to 85d3807 Compare January 9, 2025 09:07
@jasagredo jasagredo force-pushed the js/local-noindex branch 2 times, most recently from a03d515 to 19c9a86 Compare January 9, 2025 10:09
changelog.d/pr-10728 Outdated Show resolved Hide resolved
@ulysses4ever
Copy link
Collaborator

Perhaps just inline it [asPosixPath]

I'd suggest not to inline it. I'd definitely prefer reading unrelated code that calls a function like that rather than seeing the list comprehension.

@jasagredo jasagredo requested a review from philderbeast January 9, 2025 16:43
@jasagredo jasagredo added squash+merge me Tell Mergify Bot to squash-merge merge me Tell Mergify Bot to merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase and removed merge me Tell Mergify Bot to merge squash+merge me Tell Mergify Bot to squash-merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase labels Jan 13, 2025
Copy link
Contributor

mergify bot commented Jan 13, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #10728 has been dequeued. Branch protection setting 'linear history' conflicts with Mergify configuration. Branch protection setting 'linear history' works only if merge_method: squash or merge_method: rebase.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@jasagredo jasagredo removed squash+merge me Tell Mergify Bot to squash-merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days ready and waiting Mergify is waiting out the cooldown period labels Jan 13, 2025
@jasagredo
Copy link
Collaborator Author

jasagredo commented Jan 13, 2025

@hasufell pointed out that NT namespace doesn't work as I said in my comment above.

My goal here is to provide some "standard" way of declaring filepaths for a file+noindex repository on Windows. So far we have the following:

  • file+noindex://C:/foo results in C: being parsed as the Auth of the URI
  • file+noindex:///C:/foo results in the path /C:/foo which cannot be used even if normalized
  • file+noindex:////?/C:/foo results in ?/C:/foo being considered the query of the URI
  • file+noindex:////./C:/foo works as expected after normalization to \\.\C:\foo or (as Path prefixed by \\.\ is not normalised on Windows filepath#247 shows) \\.\C:/foo.

The NT mention above was just an experiment I tried to run, motivated by what I saw in the GHC User Guide, but it seems it was a misuse on my side from what Julian points out.

To be clear, I don't want to use the NT namespace in particular. I just want to use the simplest option available, which seems to be //./.

@ulysses4ever ulysses4ever added merge me Tell Mergify Bot to merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Jan 13, 2025
@ulysses4ever
Copy link
Collaborator

@mergify refresh

@haskell haskell deleted a comment from mergify bot Jan 13, 2025
Copy link
Contributor

mergify bot commented Jan 13, 2025

refresh

✅ Pull request refreshed

@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Jan 13, 2025
Copy link
Contributor

mergify bot commented Jan 13, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #10728 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:

  • label=squash+merge me

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@mergify mergify bot merged commit 529bd1f into haskell:master Jan 13, 2025
57 of 59 checks passed
@ulysses4ever
Copy link
Collaborator

@Mergifyio requeue

Copy link
Contributor

mergify bot commented Jan 13, 2025

requeue

☑️ This pull request is already queued

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Jan 13, 2025

mergify is at it again: merging before the CI finishes. We really should revert the .skip workflows sorcery. Correction: CI was green, so the bot did the right thing. Apologies on putting on the labels prematurely. I was under impression that it's done.

@jasagredo jasagredo removed merge me Tell Mergify Bot to merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days ready and waiting Mergify is waiting out the cooldown period labels Jan 13, 2025
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