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

Adapt cabal build to lts-20.26 #4203

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Conversation

jneira
Copy link
Contributor

@jneira jneira commented Jul 15, 2023

  • Since the recent upgrade of main stack build to lts-20.22 , the default constraints of cabal solver (get the newer versions) makes the build fail

  • This pr adds the required constraints, got from https://www.stackage.org/lts-20.22

  • It also ignores deprecations for unison-share-projects-api cause getting a working version of jose package is very difficult only touching cabal.project due to package revisions

  • Tested manually with cabal build all --project-file .\contrib\cabal.project -w ghc-9.2.8

  • Building with ghc-8.10.7 fails with:

[ 6 of 34] Compiling Unison.Position  ( src\Unison\Position.hs, D:\dev\ws\haskell\unison\dist-newstyle\build\x86_64-windows\ghc-8.10.7\unison-core1-0.0.0\build\Unison\Position.o )

src\Unison\Sqlite\Sql.hs:404:3: error:
    • Variable not in scope:
        asum
          :: [State.StateT [Lump] (Megaparsec.Parsec Void Text) Fragment]
             -> P Fragment
    • Perhaps you meant one of these:
        ‘sum’ (imported from Prelude),
        ‘msum’ (imported from Unison.Prelude)
    |
404 |   asum
    |   ^^^^

i think it would be not too difficult to workaround that error but i guess that build is unsupported, so 🤷

@aryairani
Copy link
Contributor

@jneira Hey thanks for the update. We are actually on lts-20.26 now; does that change anything?

@jneira
Copy link
Contributor Author

jneira commented Jul 18, 2023

I don't think so, I only checked the lts versions to help to discover the needed version bounds in cabal.project and they have not changed afaics

Copy link
Contributor

@aryairani aryairani left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@aryairani aryairani left a comment

Choose a reason for hiding this comment

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

@jneira Actually could you just add

{-# GHC_OPTIONS -Wno-deprecations #-}

to unison-share-projects-api/src/Unison/Share/API/Hash.hs instead of hiding deprecation warnings globally.

At first I was thinking it was fine as-is and we can get the deprecated call removed quickly; but just to be safe, I'd rather minimize the scope of the hidden warnings.

@jneira
Copy link
Contributor Author

jneira commented Jul 18, 2023

@aryairani sure, that was my firts idea but i was not sure if touching a source file only due to a temporary package bound was a good one.

If we think about what will happen when the stack build reach the jose version with derecations:

  • if it is only set for cabal we will hit it in stack and we will have to fix it for sure
  • if we set it for the file maybe we will miss the deprecation warning and it could be not be fixed

But as you consider, both alternatives have caveats (as usual 😝)

@jneira jneira changed the title Adapt cabal build to lts-20.22 Adapt cabal build to lts-20.26 Jul 18, 2023
@aryairani
Copy link
Contributor

@jneira Oof, I see. I was a little mixed up and didn't notice that the deprecation warning was only set in cabal.project; I assumed it was set in stack.yaml, which could then lead to even more missed deprecation warnings.

I will ask @mitchellwrosen or @tstat to weigh in, but I guess it's fine with me as-is in this case.

I also added #4212 to try to stop using Crypto.JWT.addClaim, although I think since we're an application and not a widely used library, that pinning specific dependency versions is probably the better fix for us than even having a range, which I suppose is what Stack LTS does.

Co-authored-by: Travis Staton <[email protected]>
@aryairani aryairani added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Jul 21, 2023
@mergify mergify bot merged commit e1deb78 into unisonweb:trunk Jul 21, 2023
@mergify mergify bot removed the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants