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

cabal build error, don't warn and fallback to older state. #8076

Closed
philderbeast opened this issue Mar 31, 2022 · 45 comments · May be fixed by #8077
Closed

cabal build error, don't warn and fallback to older state. #8076

philderbeast opened this issue Mar 31, 2022 · 45 comments · May be fixed by #8077

Comments

@philderbeast
Copy link
Collaborator

If I have an index-state in the project file that is newer can we please make it an error rather than a warning when cabal build is invoked?

if ts0 > isiMaxTime isi
then warn verbosity $
"Requested index-state " ++ prettyShow ts0
++ " is newer than '" ++ unRepoName rname ++ "'!"
++ " Falling back to older state ("
++ prettyShow (isiMaxTime isi) ++ ")."

-- cabal.project
packages: .
index-state: 2022-03-22T14:16:26Z
> cabal build all --enable-tests
Warning: Requested index-state 2022-03-22T14:16:26Z is newer than
'hackage.haskell.org'! Falling back to older state (2022-03-14T21:07:44Z).
Resolving dependencies..

We get a warning. It is on us to call cabal update. I'm unhappy that this is a warning and not an error as I don't think it makes sense for cabal build to proceed. Also often there is a lot of build output and it would be easy to miss this warning.

@philderbeast
Copy link
Collaborator Author

If on line 279, I change warn verbosity to error (and add 1000 years to the index-state) then the behaviour is:

> cabal build all --enable-tests
Requested index-state 3022-03-22T14:16:26Z is newer than 'hackage.haskell.org'!
Falling back to older state (2022-03-31T17:42:23Z).
CallStack (from HasCallStack):
  error, called at src/Distribution/Client/IndexUtils.hs:279:26 in
  cbl-nstll-3.7.0.0-e4be6236:Distribution.Client.IndexUtils

@Mikolaj
Copy link
Member

Mikolaj commented Mar 31, 2022

I think the codebase uses info, warn, die, die', etc. Raw error may do something different than die, though I don't remember the details.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 31, 2022

Anyway, is anybody opposed to this change? Can we think of a workflow it breaks? Should it be an option turned on by a flag somewhere?

@philderbeast
Copy link
Collaborator Author

Here's the behaviour with die' (I changed the message a little):

> cabal build all --enable-tests
Error: cabal: Requested index-state 3022-03-22T14:16:26Z is newer than
'hackage.haskell.org'! Won't optimistically fall back to older state
(2022-03-31T17:42:23Z).

@philderbeast
Copy link
Collaborator Author

@Mikolaj These commands call getSourcePackages: outdated, install, fetch, freeze, get, init, list, info.

If we change the message, could we mention the command? In the example I use "this command" because at getSourcePackages I don't know that the command is cabal build.

> cabal build
Error: cabal: Stopping this command as the requested
index-state=3022-03-22T14:16:26Z is newer than (2022-03-31T17:42:23Z), the
most recent state of 'hackage.haskell.org'. You could try 'cabal update' to
bring down a later state or request an earlier timestamp for index-state.

The same with dieNoWrap instead of die' doesn't have the same prefix of Error: cabal::

> cabal build
Error: Stopping this command as the requested index-state=3022-03-22T14:16:26Z is newer than (2022-03-31T17:42:23Z), the most recent state of 'hackage.haskell.org'. You could try 'cabal update' to bring down a later state or request an earlier timestamp for index-state.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 2, 2022

If we change the message, could we mention the command?

IMHO that makes sense.

philderbeast added a commit to cabalism/cabal that referenced this issue Apr 2, 2022
@Mikolaj Mikolaj linked a pull request Apr 2, 2022 that will close this issue
@philderbeast
Copy link
Collaborator Author

philderbeast commented Apr 3, 2022

I'd previously spelunked the code finding commands from calling functions. Now I try out the commands and some of them don't get to the fail code path; outdated, build, install, fetch, freeze, get, init, list, info

@jneira
Copy link
Member

jneira commented Apr 3, 2022

I really like the new message but i am not sure about making it an error. Maybe you dont want to jump to the new index state: the current one could work and you dont want to invalidate the cache and wait 2 hours to compile dependencies.
But you dont want to change the requested one cause you will use it eventually.
Or maybe you know nothing about the entire thing and you only want to build the package right now, without having to understand what happens and what addtional steps you have to do.

Maybe a little bit convoluted. I know you want to update your hackage index most of times but not sure if always. And make it an error will force you to make some action.

What about mention the possible consequences of not updating the index? It will make the warning larger, more annyoing and users will be eager to silent it :-P

@philderbeast
Copy link
Collaborator Author

@jneira in that case maybe we can add to the message to say that removing the index-state=... requirement will suffice to enable the build command to proceed?

@jneira
Copy link
Member

jneira commented Apr 4, 2022

@jneira in that case maybe we can add to the message to say that removing the index-state=... requirement will suffice to enable the build command to proceed?

but keeping it as an error?

then this would hold, no?:

But you dont want to change the requested one cause you will use it eventually.
Or maybe you know nothing about the entire thing and you only want to build the package right now, without having to understand what happens and what addtional steps you have to do.

I am not assuming the requested index state is something the user has set on purpose. It could be part of a given cabal.project from a random project you just want to compile. And maybe you dont want to make changes in the repo files and/or you know nothing about the index state thing.

You could do --index-state=HEAD to override the cabal.project though. Maybe we could suggest that in the error?

@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2022

How about enabling the error with an extra flag (that you can stick in you cabal.project or ~/.cabal/config)?

@jneira
Copy link
Member

jneira commented Apr 4, 2022

he, you know, there is nothing a new flag can not solve but too much extra flags 😉
i would try to avoid that if possible

@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2022

he, you know, there is nothing a new flag can not solve but too much extra flags wink i would try to avoid that if possible

Agreed. But if it's an unconditional change that breaks workflows and scripts, we need an RFC ticket, and if it confuses newbies, we need improved docs, improved messages and perhaps more.

@philderbeast
Copy link
Collaborator Author

You could do --index-state=HEAD to override the cabal.project though. Maybe we could suggest that in the error?

Tried that and it works.

@jneira
Copy link
Member

jneira commented Apr 4, 2022

Ok, lets think if we can achieve the error/warning logic using the existing config options.
What about add a value to index-state? Actual valid values are:

index-state: HEAD, unix-timestamp, ISO8601 UTC timestamp.

maybe we could add a strict:unix-timestamp and strict: ISO8601 UTC timestamp and make the warning an error in that case?

EDIT: : is already the separator between fields names and value so maybe another separator would be better

@jneira
Copy link
Member

jneira commented Apr 4, 2022

It allows put the hackage server name:

index-state:
  , hackage.haskell.org 2020-05-06T22:33:27Z
  , head.hackage 2020-04-29T04:11:05Z

so we have to be careful about the format and the parsing, to allow hackage servers containing strict for example:
strict(2020-05-06T22:33:27Z) and strict(@1474739268)? or leverage the use of @, strict@2020-05-06T22:33:27Z, strict@1474739268

Create a new flag would be cleaner in that respect and it would not affect users

@philderbeast
Copy link
Collaborator Author

Infix, the vibe I get from the current behaviour is index-state ¯\_(ツ)_/¯ 2020-05-06T22:33:27Z when I thought it would mean index-state >= 2020-05-06T22:33:27Z.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2022

OTOH, new users often never do cabal update at all, or did it 2 years ago when installing Haskell by accident. Then they promptly file error reports about cabal build or pester the particular package maintainer, so a consistent and clear messaging about the initial cabal update and the subsequent cabal updates would improve the situation.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2022

An idea: we could post an RFC to Discourse, perhaps with some initial proposal of the adoption plan, e.g., block behind a flag, first the flag is off, but printed in ~/.cabal/config and advertised and cabal init proposes to add the flag to cabal.project or set globally, unless the flag is set globally already, then after X releases, the flag is set on by default in ~/.cabal/config, etc.

@fgaz
Copy link
Member

fgaz commented Apr 4, 2022

I'm for converting the warning to an error (mentioning cabal update and the --index-state=HEAD override). It just makes more sense, just like version bounds aren't automatically relaxed if no version satisfying them exists.

I don't think we need an RFC or gradual adoption or anything like that. We are allowed to make breaking changes on major versions after all. And this is cabal-install, not Cabal

@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2022

@fgaz: I'm going to tell on you to the Stability Working Group.

@jneira
Copy link
Member

jneira commented Apr 4, 2022

well sure we are gonna break a bunch of things but next release will have so many shiny features and old bug fixes than nobody will notice :-P

@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2022

You mean, backward incompatible but working features will be the least of our worries? Quite probable, given that we have a monster release on our hands, but all the more reason to try and limit the post-release bedlam.

@hamishmack
Copy link
Collaborator

I think the warning is broken though. It seems like the intention was to only issue a warning if the fallback was unsafe (out of range), but it is issued even if the index head is after ts0. See #8086 for possible fix.

@Martinsos
Copy link
Collaborator

Martinsos commented Apr 6, 2022

My 2 cents as a first time user: After trying to use freeze file to make our builds reproducible, and learning it is not really easily possible to use it right now for that in the context of multiple platforms and team members, we decided to just fix the index-state via index-state: ... in cabal.project.

That sounded great -> practical enough for our needs and very simple. I certainly didn't expect that it will allow building with older index-state however! I thought this will ensure that exact specific index state is used. I learned about this unexpected behaviour only when @philderbeast warned me about it. And I agree that warning is not strong enough - there is so much output, I would likely never see it.

I understand it would be a breaking change to make this an error, but only for users who use index-state: ... (right?), and I am having hard time imagining that if you fixed index-state, you are ok with older index-state being used by accident. So I would rather treat it as unexpected behaviour / bug and fix it. From the stability perspective, it makes cabal behave more reproducible (and therefore stable, if we can call that also stability in some way? Hm :)) by default!

@Mikolaj
Copy link
Member

Mikolaj commented Apr 6, 2022

No objections from users so far and a couple have seen this despite no RFC. Do we know how many people have index-state in their project files? Sadly, we can't trawl Hackage for that info. Is index-state advertised in some manuals, blogposts, library readmes? If that's a relatively new or rarely used feature, I may be over-reacting.

Trawling github produces 700 hits, but perhaps most of these are forks of one repo?

https://github.com/search?q=%22index-state%22+extension%3A.project&type=Code&ref=advsearch&l=&l=

@jneira
Copy link
Member

jneira commented Apr 6, 2022

hls uses it since long time ago and i have to admit i was bitten for this not being an error. Usually you got a solver error followed by a sigh and a cabal update.
Sometimes (not much ones) being able to ignore the warning was handy though. But the --index-state=HEAD would work in those cases.
Imho converting it an error with a good suggestion on how to proceed could be even better than the actual warning.

@philderbeast
Copy link
Collaborator Author

Infix, the vibe I get from the current behaviour is index-state ¯\_(ツ)_/¯ 2020-05-06T22:33:27Z when I thought it would mean index-state >= 2020-05-06T22:33:27Z.

I was relying on intuition here and that was wrong. Asking for cabal update to satisfy index-state tempts me to think of index-state as a minimum but isn't it more like a point in time to rollback to (as described in cabal get docs)? If that is so then the request to update is being made to ensure that we have recent enough data to rollback to.

I've found two spots in the docs where index-state is described.

Project field: index-state: HEAD, unix-timestamp, ISO8601 UTC timestamp.
This allows to change the source package index state the solver uses to compute install-plans. This is particularly useful in combination with freeze-files in order to also freeze the state the package index was in at the time the install-plan was frozen.

Explaining cabal get: Downloading a package’s source
Use source package index state as it existed at a previous time. Accepts unix-timestamps (e.g. @1474732068), ISO8601 UTC timestamps (e.g. 2016-09-24T17:47:48Z), or HEAD (default). This determines which package versions are available as well as which .cabal file revision is selected (unless --pristine is used).

What does cabal update do?

cabal update
updates the state of the package index

I find cabal get the most useful but still a bit short on explanation.

Questions I have

  1. Is it a) "package index" state or b) package "index state"?
  2. When we cabal update do we get one state or the history of all possible states?
  3. When we set index-state=timestamp and there is not a state matching that exact time, is it the previous or next state that is picked?
    a. index-state <= 2020-05-06T22:33:27Z
    b. index-state >= 2020-05-06T22:33:27Z

@philderbeast
Copy link
Collaborator Author

From this comment on stackage, it looks like the answer to question 3 is a, index <= timestamp.

cabal has gained a feature called --index-state which allows you to interact with the entire package index as though it were $snapshot_date (meaning: without any new versions or revisions that occur after that date).
stackage/Add cabal.config link to snapshot pages

@fgaz
Copy link
Member

fgaz commented Apr 7, 2022

  1. "package index" state
  2. the index is not a set of states, it's just a list of cabal files ordered by publish time, setting the index state means that only packages older than that are considered
  3. you're right

@Mikolaj
Copy link
Member

Mikolaj commented Apr 7, 2022

BTW, I was convinced by the participants of the cabal dev's meeting on 7 Apr 2022 that the old behaviour, the not erroring out, was buggy, so if a user's script depends on it, it depends on a bug, so we are fine fixing the bug in a major version, with proper warnings in release notes. Let's remember to add the warnings, ideally in the very PR that fixes the bug. (Where could we put it, in addition to the normal changelog?) [Edit; and let's remember to describe how to easily get the old behaviour.]

@philderbeast
Copy link
Collaborator Author

philderbeast commented Apr 7, 2022

I haven't yet started on adding tests for this but am leaving this snippet here to later investigate and incorporate into a test.

> cabal update hackage.haskell.org,1901-03-25T14:04:30Z

> cabal build all --enable-tests
Error: cabal: Stopping this command as the requested
index-state=1901-03-25T14:04:30Z is newer than (), the most recent state of
'hackage.haskell.org'. You could try 'cabal update' to bring down a later
state or request an earlier timestamp for index-state.

> cabal update hackage.haskell.org
Downloading the latest package list from hackage.haskell.org
Package list of hackage.haskell.org is up to date at index-state 1901-03-25T14:04:30Z

> cabal build all --enable-tests
Resolving dependencies...
Build profile: -w ghc-8.10.7 -O1
...

> cabal update hackage.haskell.org,1901-03-25T14:04:30Z
Downloading the latest package list from hackage.haskell.org
Package list of hackage.haskell.org is up to date at index-state 2022-04-07T14:01:26Z

> cabal build all --enable-tests
Error: cabal: Stopping this command as the requested
index-state=1901-03-25T14:04:30Z is newer than (), the most recent state of
'hackage.haskell.org'. You could try 'cabal update' to bring down a later
state or request an earlier timestamp for index-state.

@athas
Copy link
Collaborator

athas commented May 16, 2022

I am very much in favour of this. I am often in the position of having to distribute Haskell software in source form to people, and neglecting to run cabal update is a common error.

I would even go farther and say that if index-state is present, cabal build should automatically run cabal update if needed. Pretty much all other package managers and build systems I know of do this. But that's probably a more contentious feature request, so it can wait for another time.

@pranaysashank
Copy link

pranaysashank commented Dec 18, 2023

Some projects set an arbitrary index state, for instance in haskell-language-server's cabal.project it is set to

index-state: 2023-12-13T00:00:00Z

And if you were to run it after a cabal update, you'd get the error

Warning: Requested index-state 2023-12-13T00:00:00Z is newer than
'hackage.haskell.org'! Falling back to older state (2023-12-12T23:20:00Z).

But I think it's okay in this case since it's just easy to set an index state at midnight in cabal.project instead of the exact timestamp. With the proposed change, will this become an error?

@gbaz
Copy link
Collaborator

gbaz commented Dec 18, 2023

why not then set --index-state=HEAD?

@pranaysashank
Copy link

why not then set --index-state=HEAD?

doesn't that change the behaviour though? In the previous case we are reasonably sure it's still using the same index but now we are using the new index-state and with it likely a different build plan

@gbaz
Copy link
Collaborator

gbaz commented Dec 18, 2023

I'm confused -- is this CI or manually? (I had assumed you had script updating these or something). If manually, you can just set the actual index state without much extra work. And if you want to "round it off" for aesthetic reasons, you could round it off to the prior hour, or the prior day, or whatever you like.

@pranaysashank
Copy link

Given that this warning happens after a cabal update, I have a question. Can we get a missing index later repopulated, like we have index states at A and C, once we have C can we get B in a subsequent update? Because if not then the index-state at 2023-12-13T00:00:00Z should be the same as 2023-12-12T23:20:00Z and there is no need for a message in this scenario

@gbaz
Copy link
Collaborator

gbaz commented Dec 18, 2023

I think there is confusion. The index is chronologically ordered. The index-state just is a time which we consider the index "up to". If we have index states C, then we chronologically necessarily have all prior index states. The reason that we don't want the two above to be equivalent is that it is possible a user has not run a cabal update and so the index available would not be the latest. The intended semantics is that the index-state always be a time _available from upstream hackage` and that the warning (which would now be an error) occur when there has not been an update to actually fetch that time from upstream hackage.

There was never a design consideration that people would put in a non-existent future time.

@gbaz
Copy link
Collaborator

gbaz commented Dec 18, 2023

There's actually a failure-case with the usage you describe. Suppose I set an index-state of tomorrow, and suppose everything currently builds with that, with a warning. But between now and tomorrow, now someone uploads a package to hackage which breaks things. That index-state, which worked before, no longer works. This would defeat the purpose of index-state, which is to refer to an immutable fixed set of packages. If you set a future time, then in fact you are referring to a mutable set of packages, and indeed, might as well use HEAD (i.e. you still leave open the possibility of the build plan changing).

@pranaysashank
Copy link

pranaysashank commented Dec 18, 2023

Okay that matches with my understanding, in the above example I run it after I do a cabal v2-update which means I had my index now set to hackage.haskell.org,2023-12-18T14:05:14Z. Since cabal should know they are chronologically ordered, it should not print the warning / error message.

But between now and tomorrow, now someone uploads a package to hackage which breaks things. That index-state, which worked before, no longer works

I am only referring to the situation when cabal knows the next index-state, that is in my example since I just did a cabal v2-update it knows there can be no index-states in between and that warning becomes unnecessary

@andreabedini
Copy link
Collaborator

Note that recently we have merged a PR that makes cabal error out if a requested index-state is beyond the most recent index entry.

@andreabedini
Copy link
Collaborator

It's #8944. @philderbeast does this satisfy your initial suggestion?

@philderbeast
Copy link
Collaborator Author

Yes @andreabedini, from reading the expected outputs of tests, #8944 fixes this issue. Thanks @jasagredo.

@andreabedini
Copy link
Collaborator

That's great to hear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants