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 installDirs (datadir, etc) to new-build and remove them from new-install #8556

Merged
merged 7 commits into from
Jan 4, 2023

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Oct 26, 2022

Resolves #3586

Handles the --datadir flag, etc. in new-build. Also adds that flag to those that can be parsed from cabal.project files.

Further, removes those flags, which never worked, from cabal new-install, as the new install command, by design, always places things in a fixed location in the store.

@gbaz gbaz marked this pull request as draft October 26, 2022 20:06
@gbaz gbaz marked this pull request as ready for review October 31, 2022 20:54
@jneira jneira changed the title Add installDirs (datadir, etc) to new-build Add installDirs (datadir, etc) to new-build and remove them from new-install Nov 1, 2022
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.

This seems a sensible thing to do.

@jneira
Copy link
Member

jneira commented Nov 9, 2022

nice fix, it would need a changelog, no?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 9, 2022

@gbaz: could you add a tiny changelog? thank you!

Comment on lines +175 to +177
-- install doesn't take installDirs flags, since it always installs into the store in a fixed way.
notInstallDirOpt x = not $ optionName x `elem` installDirOptNames
installDirOptNames = map optionName installDirsOptions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this how command line flags are usually filtered out? I would think we would have plenty of of cases of command foo handling some flags that are really just dispatched to the underlying command bar?

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 issue is that nixStyleOptions adds these things as options. So in this particular case we have nix style options which we specifically want to filter out from a particular v2 command.

@@ -172,7 +172,7 @@ data ProjectConfigShared
-- Things that only make sense for manual mode, not --local mode
-- too much control!
--projectConfigUserInstall :: Flag Bool,
--projectConfigInstallDirs :: InstallDirs (Flag PathTemplate),
projectConfigInstallDirs :: InstallDirs (Flag PathTemplate),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to move this, or does the comment above still apply?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 21, 2022

@gbaz: we are waiting in #8520 to test if this solves their use case. Any chance to merge this soon?

@gbaz
Copy link
Collaborator Author

gbaz commented Dec 24, 2022

Added a changelog and consider this ready to merge. Just need a second review!

@andreasabel
Copy link
Member

Needs SQUASHING.

@Mikolaj Mikolaj added the squash+merge me Tell Mergify Bot to squash-merge label Jan 2, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Jan 2, 2023

@gbaz: I've added the label in the interest of swiftness. Please feel free to change the label in the next 2 days, before it gets merged.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 2, 2023

Actually, @gbaz, I missed the merge conflict. :(

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 4, 2023
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 squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
5 participants