Skip to content

Remove support for root-cmd, fixes #3353. #3355

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

Closed
wants to merge 2 commits into from

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Apr 17, 2016

Just the last commit.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 20, 2016

As far as I can tell, Appveyor failure is spurious.

@23Skidoo
Copy link
Member

Restarted the build on AppVeyor. Also added everyone belonging to the haskell/cabal team on GitHub to my team on AppVeyor, so you should also be able to do that yourself in the future if you log in to AppVeyor with your GitHub account.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 20, 2016

Thanks, confirmed I have access now. (So everyone should be able to restart Travis and AppVeyor builds!)

@ezyang
Copy link
Contributor Author

ezyang commented Apr 21, 2016

I wonder, should we actually leave a message saying the option was removed and what a user should do instead?

@23Skidoo
Copy link
Member

That'd be nice, I think. Maybe we should also print a warning when installing with --global.

@dcoutts
Copy link
Contributor

dcoutts commented Apr 23, 2016

It's not obvious to me that it needs to be removed, but I'm not going to argue that it's important to keep. It's also not implemented in the new-build code path so one fewer thing to do :-)

It's perhaps true that cabal install --global is just always a bad plan (and certainly when executed as a user via sudo). That said, cabal configure --global [--prefix=...] is not wrong, provided one goes on to do cabal copy --dest-dir=$image or something like that. Though there's perhaps better ways to do that too, e.g. just using --prefix=.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 23, 2016

The factor which pushed me to remove is that Nix-style registration would be a bit of a pain to implement in this regime. Essentially, we need to add a new major mode to cabal-install which knows how to do a Nix style registration (as opposed to just calling Setup install --only as root, as the code does right now.) If we're willing to wade through that complexity we don't need to remove this, but that is the proximal cause for this patch.

@23Skidoo
Copy link
Member

I wonder if this can be fixed by making the Cabal library itself aware of --root-cmd instead of re-executing cabal install --only.

@dcoutts
Copy link
Contributor

dcoutts commented Apr 24, 2016

@23Skidoo I think if a root cmd exists at all then it belongs in cabal-install not Cabal lib. By rights we ought to call Setup copy --destdir= as the user and then as root only copy over the files and do the ghc-pkg register ourselves.

@dcoutts
Copy link
Contributor

dcoutts commented Apr 25, 2016

The factor which pushed me to remove is that Nix-style registration would be a bit of a pain to implement in this regime.

@ezyang so this raises a rather interesting issue actually. The current new-build code does (more or less) assume that it can control all the layout etc, so it does not currently support custom layouts of the kind you can do with Setup configure --user/--global --prefix=.... When someone is trying to construct an install image for a target deployment system, how do they go about it? They probably want /opt or /usr/local (or relocatable) or something and not the user's store paths of course. How do we accommodate this?

@ezyang
Copy link
Contributor Author

ezyang commented Apr 27, 2016

Well, the obvious and conservative choice is to make sure nix-local-build is as backwards compatible as possible, so that when we switch build to use new-build we don't wantonly break existing scripts.

@23Skidoo
Copy link
Member

23Skidoo commented May 4, 2016

So before this goes in I want to make sure that we have a plan for what to do about --global. Since we seem to agree that install --global is always a bad idea and this patch basically breaks install --global anyway, I think we should at least deprecate cabal install --global for 1.26.

We can probably also deprecate --global in other places in cabal-install, but someone needs to look into the current use cases in more detail. Currently I do not have time for that.

When it comes to Setup scripts, I think that we'll have to support the current interface indefinitely.

@ezyang
Copy link
Contributor Author

ezyang commented May 5, 2016

Looking at the option parsing code, it doesn't look like there are any facilities for deprecating flags. I do see --enable-executable-profiling is deprecated but it's done via a checkDeprecatedFlags function that is called after option parsing finishes.

@23Skidoo
Copy link
Member

23Skidoo commented May 5, 2016

Yes, flag deprecation is currently done in an ad hoc way.

@ezyang ezyang force-pushed the pr/remove-root-cmd branch from 1068042 to 4e61530 Compare May 5, 2016 05:40
@ezyang
Copy link
Contributor Author

ezyang commented May 5, 2016

OK, new patchset has a warning when you pass --root-cmd, and also deprecates --global.

@23Skidoo
Copy link
Member

23Skidoo commented May 5, 2016

Great, let's merge then.

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.

3 participants