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

Switch to data-default from data-default-class #1852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Oleksandr-Zhabenko
Copy link

Switched to data-default instead of data-default-class. Fixed #1850

@Oleksandr-Zhabenko Oleksandr-Zhabenko changed the title Switched to data-default instead of data-default-class. Fixed https://github.com/yesodweb/yesod/issues/1850 Switched to data-default instead of data-default-class. Fixed https://github.com/yesodweb/yesod/issues/1850 Oct 28, 2024
@jezen jezen changed the title Switched to data-default instead of data-default-class. Fixed https://github.com/yesodweb/yesod/issues/1850 Switch to data-default from data-default-class Oct 28, 2024
Copy link
Member

@jezen jezen left a comment

Choose a reason for hiding this comment

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

@Oleksandr-Zhabenko FYI, this is failing to build.

@Oleksandr-Zhabenko
Copy link
Author

Oleksandr-Zhabenko commented Nov 21, 2024

@Oleksandr-Zhabenko FYI, this is failing to build.

@jezen this is due to the following.

Stackage still is missing the newer version that will completely resolve the issue. It is present only on Hackage. Since the files changed are all .cabal files, I thought that it would update the needed versions from the Hackage, but it does not. It looks like it is needed to use conditional builds in the .cabal files and CPP extensions in the source files to check the package versions.

@Oleksandr-Zhabenko
Copy link
Author

But if we use CPP and conditional .cabal files then the issue #1850 is still actual, because hledger still uses data-default package, which is preferred in the future, instead of data-default-class.

@Oleksandr-Zhabenko
Copy link
Author

As thielema said, this change is for the future when the data-default-0.8 is available on Stackage.

@Oleksandr-Zhabenko
Copy link
Author

But wait, stack has functionality to deal with such cases.

@Oleksandr-Zhabenko
Copy link
Author

Oleksandr-Zhabenko commented Nov 21, 2024

These two commits
seem to resolve the issue.

@Oleksandr-Zhabenko
Copy link
Author

@jezen To use just one commit,
this one is done.

@jezen
Copy link
Member

jezen commented Nov 22, 2024

There are a couple of build failures on ubuntu-latest, --resolver lts-22.

One is like this.

tls                          > /tmp/stack-1603e7557b70df64/tls-1.8.0/Network/TLS/Parameters.hs:417:39: error: [GHC-39999]
tls                          >     • No instance for ‘Default ValidationCache’
tls                          >         arising from a use of ‘def’
tls                          >     • In the ‘sharedValidationCache’ field of a record
tls                          >       In the expression:
tls                          >         Shared
tls                          >           {sharedCredentials = mempty,
tls                          >            sharedSessionManager = noSessionManager, sharedCAStore = mempty,
tls                          >            sharedValidationCache = def, sharedHelloExtensions = []}
tls                          >       In an equation for ‘def’:
tls                          >           def
tls                          >             = Shared
tls                          >                 {sharedCredentials = mempty,
tls                          >                  sharedSessionManager = noSessionManager, sharedCAStore = mempty,
tls                          >                  sharedValidationCache = def, sharedHelloExtensions = []}
tls                          >     |
tls                          > 417 |             , sharedValidationCache = def
tls                          >     |                                       ^^^

The other is like this.

yesod-form-multi             > /home/runner/work/yesod/yesod/yesod-form-multi/Yesod/Form/MultiInput.hs:38:0: error:
yesod-form-multi             >      warning: extra tokens at end of #ifdef directive
yesod-form-multi             >        38 | #ifdef MIN_VERSION_shakespeare(2,0,18)
yesod-form-multi             >           | 
yesod-form-multi             >    |
yesod-form-multi             > 38 | #ifdef MIN_VERSION_shakespeare(2,0,18)
yesod-form-multi             >    | ^

Do you think you can fix these? Happy to support you on this.

@Oleksandr-Zhabenko
Copy link
Author

Oleksandr-Zhabenko commented Nov 22, 2024

The changes needed to be done for the correction of the first one is related to tls-2.1.2 package that should be used instead of tls-1.8.

But this leads to the following error:

connection > /tmp/stack-233a16a5e48d1819/connection-0.3.1/Network/Connection.hs:100:33: error:
connection > Not in scope: data constructor ‘TLS.SessionManager’
connection > Perhaps you meant variable ‘TLS.noSessionManager’ (imported from Network.TLS)
connection > Neither ‘Network.TLS’ nor ‘Network.TLS.Extra’ exports ‘SessionManager’.
connection > |
connection > 100 | connectionSessionManager mvar = TLS.SessionManager
connection > | ^^^^^^^^^^^^^^^^^^

And the connection-0.3.1 is the last available version, and their source repository is archived on GitHub by the author.
https://github.com/vincenthz/hs-connection
It is needed to ask the maintainer Vincent Hanquez [email protected] to at least add revision to the cabal file on Hackage, which includes tls >= 2.1.2 for the code to compile.

@Oleksandr-Zhabenko
Copy link
Author

As for the CPP preprocessing macros, I think it is better to delete the ifdef part and use just the following if-else.

@Oleksandr-Zhabenko
Copy link
Author

Oleksandr-Zhabenko commented Nov 22, 2024

I have just tried to resolve the non-building cases, this led to updating many dependencies in the stack.yaml file on the project level. But still, the connection package is not compiled because it has no appropriate version, it is needed to contact @vincenthz to provide some workaround for this or to fork his archived repository and then try to add changes from there to the Hackage package connection.

@psibi
Copy link
Member

psibi commented Nov 23, 2024

And the connection-0.3.1 is the last available version, and their source repository is archived on GitHub by the author.
https://github.com/vincenthz/hs-connection

Can you switch to this maintained fork instead: https://hackage.haskell.org/package/crypton-connection ?

@Oleksandr-Zhabenko
Copy link
Author

And the connection-0.3.1 is the last available version, and their source repository is archived on GitHub by the author.
https://github.com/vincenthz/hs-connection

Can you switch to this maintained fork instead: https://hackage.haskell.org/package/crypton-connection ?

It seems to be resolved. I will now squash commits to have just one final with the needed changes.

@psibi
Copy link
Member

psibi commented Nov 25, 2024

I see that there are still some CI errors. Probably we can remove the old resolver. I think having CI for the last 3 GHC major versions should be good enough.

@Oleksandr-Zhabenko
Copy link
Author

Oleksandr-Zhabenko commented Nov 25, 2024

I see that there are still some CI errors. Probably we can remove the old resolver. I think having CI for the last 3 GHC major versions should be good enough.

I have the same opinion. The fails are for the lts-16 resolver only, and are connected with base-4.13* and basement-0.14* series instead of the provided newer versions.

Fixed issue with dependencies for data-default branch

Resolved non-building cases using crypton-connection

Resolved non-building cases by dropping support to lts-16
Copy link
Author

@Oleksandr-Zhabenko Oleksandr-Zhabenko left a comment

Choose a reason for hiding this comment

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

Now it builds.

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.

instance Default WidgetFileSettings vanished
3 participants