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

Musical chairs to switch `new-` and `old-` #5358

Merged
merged 1 commit into from Jun 12, 2018

Conversation

@typedrat
Copy link
Collaborator

@typedrat typedrat commented Jun 2, 2018

This is the start of work to make the current new- commands the standard versions and to make the current commands have the old- prefix. Of course, to have this, we have to decide what we are willing to give up commands-wise and what we want to wait on being ready. clean and sdist are the two most important I think, of the ones that are still out.

Please 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.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

@quasicomputational
Copy link
Contributor

@quasicomputational quasicomputational commented Jun 2, 2018

Is it worth splitting into two commits, do you think, with one creating the old-* aliases, and another dropping the new- prefix? We probably want to apply the first of those anyway, regardless of when the second is ready to happen, and it makes falling back to the old codepaths less painful if you're testing on multiple Cabal versions than having to check cabal --version.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 2, 2018

@phadej
Copy link
Collaborator

@phadej phadej commented Jun 2, 2018

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 3, 2018

Okay, that's a fair point. I went with old because I couldn't think of another prefix that couldn't almost be taken as a positive (if not now, once the new prefix goes away). one almost seems like it's a unified feature, not a semi-deprecated one.

I can kill this and replace it with just adding old prefixes, since yeah we do need to accommodate people using HEAD in production. Not sure how long to wait for the switch-over, though.

@BardurArantsson
Copy link
Collaborator

@BardurArantsson BardurArantsson commented Jun 5, 2018

@phadej

As there are people using cabal-install-head in production like environments,

Anything that happens as a consequence of that is on them.

If they're not using fixed snapshots then they're begging for instability and for their infrastructure and/or builds to be "owned". A lot of people have the ability to push directly to Cabal master at this point without any oversight except someone noticing after the fact. (I'm sure most of them are very trustworthy, but if you're running directly from HEAD in production -- rather than stable hand-picked snapshots -- you need ALL of them to be 100% trustworthy and 100% conscientious about how they handle their GitHub SSH keys, etc. etc.)

That said, I think I agree that's its worth starting by first introducing old-* aliases so that people who are worried about new-* can start transitioning to those before The World Changes. (However, that's only the case if the intent is to keep those old-* aliases around for at least one release... which I presume it is?)

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 5, 2018

@typedrat typedrat closed this Jun 6, 2018
@typedrat typedrat force-pushed the typedrat:musical-chairs branch from 08c942b to d6b829d Jun 6, 2018
@typedrat typedrat reopened this Jun 9, 2018
@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 9, 2018

Should now follow the agreed-upon path for moving this forward.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 9, 2018

Unless anyone has any concerns or desired changes (and assuming that my test fix worked), I think this is ready to be merged.

@typedrat typedrat moved this from In Progress to Done in Have new-build become build (GSOC2018) Jun 9, 2018
@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 10, 2018

The Appveyor failure seems to be unrelated (unless adding the aliases somehow is making it not get pkg-config)

@@ -3,7 +3,9 @@ Nix-style Local Builds

Nix-style local builds are a new build system implementation inspired by Nix.
The Nix-style local build system is commonly called "new-build" for short after the ``cabal new-*`` family of commands that control it.
However those names are only temporary until Nix-style local builds becomes the default.
However, those names are only temporary until Nix-style local builds becomes the default. This is expected to happen soon. For those who
do not wish to use the new functionality, the classic project style will not be removed, but these legacy commands will require the usage

This comment has been minimized.

@quasicomputational

quasicomputational Jun 10, 2018
Contributor

Should probably say 'will not be removed immediately', since we do plan to get rid of them at some point.


import qualified Data.Text as T

-- Duplicated code, but makes things cleaner and lets me keep how this happens a dirty little secret.

This comment has been minimized.

@quasicomputational

quasicomputational Jun 10, 2018
Contributor

Duplicated from where? Worth saying that, and having a note in its original home about how any changes should be reflected here as well.


"Please switch to using either the new project style or the legacy v1- aliases,\n" ++
"as new-style projects will become the default in the next version of\n" ++
"cabal-install."

This comment has been minimized.

@quasicomputational

quasicomputational Jun 10, 2018
Contributor

Would it be worth explicitly asking users to file bugs in this message if there's anything they can't do with the new- commands that was working before?

, hiddenCmd uninstallCommand uninstallAction
, hiddenCmd formatCommand formatAction
, hiddenCmd upgradeCommand upgradeAction
, hiddenCmd win32SelfUpgradeCommand win32SelfUpgradeAction
, hiddenCmd actAsSetupCommand actAsSetupAction
, hiddenCmd manpageCommand (manpageAction commandSpecs)

This comment has been minimized.

@quasicomputational

quasicomputational Jun 10, 2018
Contributor

Trailing whitespace.


deprecationNote :: String
deprecationNote =
"This command is a part of the legacy v1 style of cabal usage.\n\n" ++

This comment has been minimized.

@quasicomputational

quasicomputational Jun 10, 2018
Contributor

Rather than saying "This command", better to name the command being used, to save digging in complicated scripts.

, commandUsage = updateMsg . commandUsage
, commandDescription = (updateMsg .) <$> commandDescription
, commandNotes = Just $ \pname -> case commandNotes of
Just notes -> updateMsg (notes pname) ++ "\n" ++ deprecationNote

This comment has been minimized.

@quasicomputational

quasicomputational Jun 10, 2018
Contributor

This is achieving too much:

cabal new-run cabal -- v1-install --help

...

Examples:
  cabal v1-install                     Package in the current directory
  cabal v1-install foo                 Package from the hackage server
  cabal v1-install foo-1.0             Specific version of a package
  cabal v1-install 'foo < 2'           Constrained package version
  cabal v1-install v1-haddock --bindir=$HOME/hask-bin/ --datadir=$HOME/hask-data/
                                    Change v1-installation destination

I think you're going to have to put the v1 in each legacy command's commandNotes, and then strip it out in the legacyUI (rather than adding it in the aliasedUI and keeping the commandNotes as they are).

@23Skidoo
Copy link
Member

@23Skidoo 23Skidoo commented Jun 10, 2018

AppVeyor seems to be flaky lately.

@typedrat typedrat dismissed quasicomputational’s stale review Jun 10, 2018

Satisfied requests.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 10, 2018

If these lights turn green (or at least, don't turn red in a way that looks like my fault) I'll squash and merge this tonight.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 11, 2018

I don't understand what's causing the failures.

It's like it's running against an old revision of the tests, because the expected output files match what it's claiming is wrong.

@ezyang
Copy link
Contributor

@ezyang ezyang commented Jun 11, 2018

@typedrat, I'm not sure how you determined the expected output files, but the cabal run commands in this test are specified with -v0, so you shouldn't be seeing any of the warning output in the expected output transcript. Can you maybe run this test with verbose output on Linux, and paste it here?

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 11, 2018

-v0 doesn't turn off the messages right now, since verbosity is for some reason not a global flag so there's no way to uniformly get it as it stands. That's an easy fix, though.

@ezyang
Copy link
Contributor

@ezyang ezyang commented Jun 11, 2018

My guess is that the verbosity flag is being non-uniformly applied depending on if you're in Linux or Windows. I guess, if you are not in the mood for sussing out what platform specific difference is causing the problem, you can suppress it by removing recordMode RecordAll and instead grepping the stdout for the expected text.

@quasicomputational
Copy link
Contributor

@quasicomputational quasicomputational commented Jun 11, 2018

Over-replacement is still happening:

$ cabal new-run cabal -- v1-build --help
Up to date
Compile all/specific components.

Usage: cabal v1-build [FLAGS]
   or: cabal v1-build COMPONENTS [FLAGS]

Components encompass v1-executables, v1-tests, and v1-benchmarks.
$ cabal new-run cabal -- v1-test --help
Up to date
Run all/specific tests in the test suite.

Usage: cabal v1-test [FLAGS]
   or: cabal v1-test TESTCOMPONENTS [FLAGS]

If necessary (re)v1-configures with `--enable-v1-tests` flag and v1-builds the v1-test
suite.

Remember that the v1-tests' dependencies must be v1-installed if there are
additional ones; e.g. with `cabal v1-install --only-dependencies --enable-v1-tests`.

By defining UserHooks in a custom Setup.hs, the package can define actions to
be v1-executed before and after v1-running v1-tests.
@23Skidoo
Copy link
Member

@23Skidoo 23Skidoo commented Jun 11, 2018

If necessary (re)v1-configures with `--enable-v1-tests` flag and v1-builds the v1-test
suite.

That's a funny one.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 11, 2018

Huh, I'm not sure how to proceed.

I guess if I gotta just bite the bullet and do it all by hand, I can. That way, I can have it strip the v1-s, and that's a lot more fool-proof. Alternatively, could just switch it up in such a way that it only replaces if there are no alphanumeric characters touching it, so it'll handle symbols but not the naïve solution we have now.

@quasicomputational
Copy link
Contributor

@quasicomputational quasicomputational commented Jun 11, 2018

I think putting v1- in the canonical description and stripping it out has to be the way to go; the other approach sounds too complicated and will probably still fail in weird ways unless you do something like mark up the description with 'this is a command name' and 'this isn't' information, and that's not less work than the first option.

@typedrat typedrat force-pushed the typedrat:musical-chairs branch from 6488a2f to 8c8ec5a Jun 11, 2018
@quasicomputational
Copy link
Contributor

@quasicomputational quasicomputational commented Jun 12, 2018

Another (minor) UI problem: the same warning is printed for both the v1- and unprefixed versions:

The (v1-)build command is a part of the legacy v1 style of cabal usage.

Please switch to using either the new project style or the legacy v1- aliases,
as new-style projects will become the default in the next version of
cabal-install. Please file a bug if you cannot replicate a working v1- use
case with the new-style commands.

But, if the user's invoked v1-build, we don't need to tell them to switch to explicit prefices, and we can probably reduce it down to just a nag that it'll be removed eventually and they should move over to new-style commands. The full warning should stay for build, of course.

CI failures look legit, but it looks like a technicality and not something fundamental, hopefully?

These issues aside I think that this looks good and, with a little more work, should be ready to merge soon.

commandDescription = Just $ \_ -> wrapText $
"Components encompass executables, tests, and benchmarks.\n"
++ "\n"
++ "Affected by configuration options, see `v1-configure`.\n",

This comment has been minimized.

@quasicomputational

quasicomputational Jun 12, 2018
Contributor

I think the convention is to use straight quotes? This occurs a bunch so you'll have to search-and-replace.

This comment has been minimized.

@typedrat

typedrat Jun 12, 2018
Author Collaborator

That's just copy-pasted from Distribution.Simple.Setup so if it's inconsistent that's not on me. 😛

This comment has been minimized.

@quasicomputational

quasicomputational Jun 12, 2018
Contributor

Oh, fair point. I do see we're inconsistent today. I've opened #5377 as a TODO item about it.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 12, 2018

Fixed the CI failures. Forgot about one of the places that things get special cased based on what command is used.

@typedrat
Copy link
Collaborator Author

@typedrat typedrat commented Jun 12, 2018

Appveyor has the demons again and Travis ran out of memory once, but otherwise it looks good.

@typedrat typedrat force-pushed the typedrat:musical-chairs branch from df56c7c to c7cf8e2 Jun 12, 2018
@typedrat typedrat merged commit 15e793a into haskell:master Jun 12, 2018
13 checks passed
13 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux-7.10.3 Downstream Travis
Details
linux-7.4.2 Downstream Travis
Details
linux-7.6.3 Downstream Travis
Details
linux-7.8.4 Downstream Travis
Details
linux-8.0.2 Downstream Travis
Details
linux-8.2.2 Downstream Travis
Details
linux-8.4.3 Downstream Travis
Details
linux-8.4.3-fdebug-expensive-assertions Downstream Travis
Details
osx-7.10.3 Downstream Travis
Details
osx-7.8.4 Downstream Travis
Details
osx-8.0.2 Downstream Travis
Details
@typedrat typedrat changed the title [WIP] Musical chairs to switch `new-` and `old-` Musical chairs to switch `new-` and `old-` Jun 13, 2018
@lspitzner
Copy link
Collaborator

@lspitzner lspitzner commented Jun 13, 2018

Does this not unnecessarily break any existing scripts that expect (old) semantics, if "cabal" switches? How about creating a new executable with a somewhat close name (canoe?) that exposes just the new-style semantics, without the prefix? Or is this the plan all along? (What is the plan? I see no issues or other motivation linked for this PR..)

I get that a new executable name creates friction, but the alternative seems to force users and scripts to check "cabal --version" way too much in the foreseeable future, which certainly is friction too, and much worse.

@23Skidoo
Copy link
Member

@23Skidoo 23Skidoo commented Jun 13, 2018

I don't think that we should have eternal backwards compatibility in the command-line UI. We want everyone to migrate to new-build after all.

I get that a new executable name creates friction, but the alternative seems to force users and scripts to check "cabal --version" way too much in the foreseeable future, which certainly is friction too, and much worse.

The alternative for users who want legacy semantics is to either use an old version of cabal-install or replace cabal CMD with cabal v1-CMD in their scripts. There will be time (at least one release cycle) for everyone to update.

@gbaz
Copy link
Collaborator

@gbaz gbaz commented Jun 13, 2018

Just for reference, I know we have had a number of prior discussions, though I don't remember where they are all located, but the idea that new-build will eventually become build goes back quite some time (to the beginning of this project even). Here is an announcement making reference to it in 2016: http://blog.ezyang.com/2016/05/announcing-cabal-new-build-nix-style-local-builds/

This work is being done as part of a gsoc project for the purpose of making that switch possible, as described on the ideas page here: https://summer.haskell.org/ideas.html#cabal-new-build and in the accepted proposals page here: https://summer.haskell.org/news/2018-04-23-accepted-projects.html#helping-cabal-new-build-become-just-cabal-build

(note that this switch, when it occurs, will involve a major version bump).

@lspitzner
Copy link
Collaborator

@lspitzner lspitzner commented Jun 13, 2018

I don't think that we should have eternal backwards compatibility in the command-line UI.

The old executable can easily be phased out after a while. I don't see what would be particularly eternal about this.

We want everyone to migrate to new-build after all.

And if the old executable is phased out after a while, this forces them to switch, only without also breaking scripts and UI expectation in a surprising fashion, and creating confusion during a lengthy transition period.

If my script stops working because there is no "cabal" any longer, I can easily tell what is up. If it breaks in some fashion because commands switched semantics this is much more confusing.

The same for any haskell tutorials, project installation instructions, and other sorts of user-facing docs.

The alternative for users [..] is to either use an old version of cabal-install or replace cabal CMD with cabal v1-CMD in their scripts

This simply does not address the question of which approach creates less friction.

I am sure I miss some of the disadvantage that come with a new executable name. Feel free to address those.

@lspitzner
Copy link
Collaborator

@lspitzner lspitzner commented Jun 13, 2018

a bit more explicitly, an explanation what i have in mind:

command            semantics
------------------------------------------------------------
cabal build        old
cabal new-build    new
cabal old-build    [does not exist]
cabal v1-build     [does not (need to) exist]
canoe build        new
canoe old-build    [does not exist]
canoe new-build    [does not exist]
canoe v1-build     [does not (need to) exist]

and no need to drastically change existing semantics. The whole cabal executable can be phased out in a few releases, leaving only canoe.

This approach almost comes naturally when you consider the meaning of the parts of this invocation:

cabal v1-build
|     |   `which function exactly?
|     `which general mode of operation?
`which build tool?

where the only of these three that changes regularly when working on any given project is the third, so it makes much more sense to write this as

cabal-v1 build

and the trivial conclusion is to just rename the executable, and encode build-tool+operation-mode in its name.

@lspitzner
Copy link
Collaborator

@lspitzner lspitzner commented Jun 13, 2018

and sorry if this PR is the wrong place to discuss this, but the links @gbaz provided don't exactly feel more appropriate for such feedback.

@23Skidoo
Copy link
Member

@23Skidoo 23Skidoo commented Jun 13, 2018

The old executable can easily be phased out after a while. I don't see what would be particularly eternal about this.

You suggestion is that we should never be allowed to reuse the cabal name, i.e. the semantics of cabal build should be eternally fixed to v1-build because of backwards compat considerations.

@lspitzner
Copy link
Collaborator

@lspitzner lspitzner commented Jun 13, 2018

You suggestion is that we should never be allowed to reuse the cabal name, i.e. the semantics of cabal build should be eternally fixed to v1-build because of backwards compat considerations.

If, as I propose, we phase out "cabal", its semantics change from "old" to "does not exist". I maintain that this is not eternal.

@lspitzner
Copy link
Collaborator

@lspitzner lspitzner commented Jun 13, 2018

But yes, I think it would be rather useful if I was able to tell with certainty the intended meaning of a "cabal build" invocation I will find in some old build script at any point in the future.

@andreabedini
Copy link

@andreabedini andreabedini commented Jul 11, 2018

I agree with @lspitzner and I made a similar comment in #5399 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.