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

Add file dependencies to Git files #199

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

Conversation

guaraqe
Copy link
Contributor

@guaraqe guaraqe commented Aug 28, 2023

This adds file dependencies to Git files in the TH code we had for getting versions. By itself, it didn't work as I expected. I wanted:

  • Do a commit
  • Package recompiles to update version

However, if we run with --force-dirty, it seems to work well. That is, one should be able to choose when the version should reload or not.

To test:

  • Run stack run unbeliever:exe:experiment --force-dirty -- --version, and see the current version
  • Do some commit which does not change the Haskell code.
  • Run stack run unbeliever:exe:experiment --force-dirty -- --version again. The version should be different, and compatible with the new git commit.

I left some comments in the code below.

@@ -70,7 +70,7 @@ executable experiment
, core-text
, prettyprinter
, unordered-containers
buildable: False
buildable: True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just for testing, we can remove it after.

@@ -98,7 +98,7 @@ program = do
info "Brr! It's cold"

version :: Version
version = $(fromPackage)
version = $$fromPackage
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the package use the same system as githash itself, see the change in type signature in fromPackage.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a breaking change? Every single program built with this library uses fromPackage in its current form.

Comment on lines +1 to +2
resolver: lts-21.9
compiler: ghc-9.4.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the Stack snapshot.

@istathar
Copy link
Member

@TheWizardTower do you think passing --force-dirty is the best workaround here? I'm pretty impressed that Juan figured this out (and it's clearly better than smacking it with stack clean cause man that's been wrecking our build coaches!) but I'd value a second opinion.

@istathar
Copy link
Member

I think your approach is really good, and on reflection having to --force-dirty isn't the worst inconvenience. So I'd like to merge this!

Changing from fromPackage :: Q Exp to fromPackage :: Code Q Version, however, really rather breaks things. I'll have a more detailed try tomorrow and see what I can come with.

Copy link
Member

@istathar istathar left a comment

Choose a reason for hiding this comment

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

Do you want to continue with this patch's direction, @guaraqe? I think your insight to use addDependentFile is excellent and either way we will include these commits in the code base!

Left _ -> pure Nothing
Right value -> do
runIO $ print (giFiles value)
addDependentFile "/home/juan/lol"
Copy link
Member

Choose a reason for hiding this comment

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

Heh, we need to patch this out :)

-- but that's not happening. So more voodoo TH nonsense instead.

[e|result|]
fromPackage :: Code Q Version
Copy link
Member

Choose a reason for hiding this comment

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

We need a find to return this to being in the Q monad. I like that it returns a Version (far more explicit that :: Q Exp) but I don't want to break all the applications that do

version = $(fromPackage)

right now.

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.

2 participants