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

Add --project-dir flag #8454

Merged
merged 1 commit into from
Mar 1, 2023
Merged

Add --project-dir flag #8454

merged 1 commit into from
Mar 1, 2023

Conversation

cydparser
Copy link
Collaborator

@cydparser cydparser commented Sep 5, 2022

Details

Given the following directory structure:

/opt
|-- proj
    |-- cabal.project
    |-- nix/
        |-- cabal.project
    |-- lib/

Where:

  • proj is the project root directory
  • lib contains a Cabal file
  • nix contains Nix-specific files

Behavior in 3.8.1.0

current directory --project-file resolved root notes
1 any /opt/proj/cabal.project /opt/proj
2 /opt/proj/* cabal.project /opt/proj
3 /opt/proj/* nix/cabal.project /opt/proj
4 /opt/ proj/cabal.project /opt error
5 /opt/proj/lib/ ../cabal.project /opt/proj/lib error

The last two scenarios result in an error after dist-newstyle is created in
the erroneously resolved root directories.

Behavior in this PR

  • Deprecate using --project-file with an absolute path
  • Add --project-dir
current directory --project-dir --project-file resolved root notes
1 any /opt/proj/cabal.project /opt/proj warning
2 /opt/proj/* cabal.project /opt/proj
3 /opt/proj/* nix/cabal.project /opt/proj
4 /opt/ proj/cabal.project /opt error
5 /opt/proj/lib/ ../cabal.project /opt/proj/lib error
6 /opt/ proj /opt/proj
7 /opt/proj/lib/ ../ /opt/proj

This is the conservative option: it is additive without changing the original
behavior. Scenarios 4 and 5 are still errors, but we could detect the misuse of
--project-file by looking for distDirectory and add a warning that suggests
using --project-dir.

Alternative proposal

Use the behavior of #7749.

  • Allow using an absolute path with --project-file
  • Project root will either be --project-dir or the directory containing --project-file

When both --project-dir and --project-file are specified, and
--project-file is a relative path, --project-file will be resolved relative
to --project-dir (rather than to the current working directory).

current directory --project-dir --project-file resolved root notes
1 any /opt/proj/cabal.project /opt/proj
2 /opt/proj/* cabal.project /opt/proj
3 /opt/proj/* nix/cabal.project /opt/proj/nix error
4 /opt/ proj/cabal.project /opt/proj
5 /opt/proj/lib/ ../cabal.project /opt/proj
6 any /opt/proj /any-path/cabal.project /opt/proj
7 any /opt/proj nix/cabal.project /opt/proj

Scenario three becomes an error, making this a breaking change, but the behavior
can be recovered by specifying --project-dir.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 5, 2022

It looks like a really comprehensive PR. I don't remember, is there a near-consensus to use this approach in the tickets you mention? Or do you plan to initiate discussion once the PR is ready? I'm asking to avoid a situation where you sit tight on a perfectly good PR, while people bemoan the lack of progress in the other tickets. :)

@ulysses4ever
Copy link
Collaborator

This looks like a new approach to me, but a fairly clear and helpful one. I suggest to put forward a note that specifying --project-dir doesn’t require you to have an actual cabal.project — that was one of the complaints about --project-file (#7940 (comment)). I see that it seems to hold judging by your comments here and there, but it’s never spelled out clearly.

@cydparser
Copy link
Collaborator Author

@Mikolaj

is there a near-consensus to use this approach in the tickets you mention?

No, this approach was:

Or do you plan to initiate discussion once the PR is ready?

I'm hoping that this PR can serve as the forum for discussion.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 5, 2022

I'm hoping that this PR can serve as the forum for discussion.

Splendid. In that case, could you drop a note in the relevant tickets?

@cydparser
Copy link
Collaborator Author

I've added more details and an alternative proposal to the description. If others like the alternative better, I would be happy to implement the change.

@Mikolaj Mikolaj added type: discussion type: RFC Requests for Comment labels Nov 23, 2022
@pranaysashank
Copy link

I think the alternative scenario is better (if the breaking change in scenario 3 is acceptable) as it works with an absolute path as well and fixes the relative path behaviour (this is the main reason for the existence of the other PR, see this comment by @fgaz for an explanation of the relative behaviour). So the new situation will be,

  • project root is the directory where --project-file is located in unless --project-dir is specified.

I wonder if one of the motivations mentioned (#7695 (comment)) is now better solved with imports in cabal.project.

If the breaking behaviour is not acceptable and we want to just add the new --project-dir flag, I don't see why we would want to deprecate support for absolute paths when relative behaviour is definitely a bug

@cydparser
Copy link
Collaborator Author

I wonder if one of the motivations mentioned (#7695 (comment)) is now better solved with imports in cabal.project.

Unfortunately not (unless I'm mistaken). How would one conditionally import a file based on whether or not the user is building with Nix? Can one check for the presence of an environmental variable in an if-clause condition?

@Mikolaj
Copy link
Member

Mikolaj commented Jan 9, 2023

I think the RFC period is over. I propose to review and, if reviews are positive, merge to be released in cabal 3.10 and then collect feedback from users of 3.10.

@Mikolaj Mikolaj removed type: discussion type: RFC Requests for Comment labels Jan 9, 2023
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

I don't have the relevant user experience to judge the usability impact, but as a software contribution the PR is very solid and I trust the author's user experience.

cabal-install/src/Distribution/Client/ProjectConfig.hs Outdated Show resolved Hide resolved
@Mikolaj Mikolaj added the re: project-file Concerning cabal.project files label Jan 9, 2023
@jneira
Copy link
Member

jneira commented Jan 9, 2023

Hi, i only want to note that i am not sure if changes has enough integration tests: i see a great unit test case but it is not clear to me changes in integration ones covers main cases of the new functionality

@Kleidukos
Copy link
Member

An integration test can certainly help clarifying the expected behaviour of the patch.

@cydparser
Copy link
Collaborator Author

I've (hopefully) made all requested changes.

@Kleidukos
Copy link
Member

@cydparser Fantastic, thank you very much! I'll let you set the "squash+merge me" label on this PR and the merge train will take it from here. :)

@cydparser cydparser added the squash+merge me Tell Mergify Bot to squash-merge label Feb 23, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 26, 2023
@cydparser cydparser added merge me Tell Mergify Bot to merge and removed squash+merge me Tell Mergify Bot to squash-merge labels Mar 1, 2023
@cydparser
Copy link
Collaborator Author

Mergify failed with an obscure message. I manually rebased and squashed the commits into a single commit. "Validate macos-latest ghc-8.4.4" failed after running over 5 hours.

@ulysses4ever
Copy link
Collaborator

@cydparser I'm sorry that you have to deal with CI falkiness, but it is only that and nothing else. Let's try once again (I restarted failing jobs).

@Mikolaj
Copy link
Member

Mikolaj commented Mar 1, 2023

Yes, OSX runners sometimes do that. No idea. Potentially just GHA flakiness. The solution is stop the job and restart it.

@mergify mergify bot merged commit 42a03ff into haskell:master Mar 1, 2023
@ulysses4ever
Copy link
Collaborator

Too bad this was merged just days after 3.10 was frozen (informally, but so I heard)...

@Mikolaj
Copy link
Member

Mikolaj commented Mar 2, 2023

Yes, unfortunately, But to clarify, we were in semi-freeze mode for some time already and for the last week or two we were only merging 3.8 and 3.9 regression fixes (and doc and CI tweaks). I failed to announce that on the IRC channel, probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: project-file Concerning cabal.project files
Projects
None yet
6 participants