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

SCP-1066 reorganizing plc-agda #2254

Merged
merged 13 commits into from
Aug 24, 2020
Merged

SCP-1066 reorganizing plc-agda #2254

merged 13 commits into from
Aug 24, 2020

Conversation

jmchapman
Copy link
Contributor

@jmchapman jmchapman commented Aug 20, 2020

  • reduce number of times the list of modules appears in the cabal file

The list appears in a common stanza. It is listed twice as everthing in autogen-modules has to appear again in other-modules.

Created a library in the cabal file and a common stanza for the usual stuff you put in a common stanza.

  • Split into a library and executable (not 100% sure this will work)

  • Sort out naming of components

  • Potentially refactor existing plc-agda tests to call plutus-core functions directly rather than calling out to plc.

Arguing with CI expected.

@michaelpj
Copy link
Contributor

To be clear, I'm anticipating that you can put the big list of modules only in the library, and then have the executable just depend on the library.

@jmchapman
Copy link
Contributor Author

I tried that but concluded that it doesn't help.
See here: https://cabal.readthedocs.io/en/3.4/cabal-package.html#example-a-package-containing-a-library-and-executable-programs

The executable has to repeat all the modules that it uses, which in this case is all of them.

@michaelpj
Copy link
Contributor

I don't think so... Have a look at the stanza for the plc exectuable in plutus-core: it uses modules from the main library, but doesn't have to repeat them all. It just declares a dependency on the library and then uses it like a normal package.

@jmchapman
Copy link
Contributor Author

I tried this out with a small 'hello world' example in Agda. Also, I think there may be another problem, I just tried adding a library to plc-agda.cabal and it doesn't appear to be allowed:

$ cabal v2-build metatheory
Warning: Requested index-state2020-08-05T00:00:00Z is newer than
'hackage.haskell.org'! Falling back to older state (2020-08-04T23:45:27Z).
Resolving dependencies...
Error:
    Internal libraries only supported with per-component builds.
    Per-component builds were disabled because build-type is Custom
    In the inplace package 'plc-agda-0.1.0.0'

@michaelpj
Copy link
Contributor

So, there's a difference between having an "internal library" and a library component at all. In particular, the "main" library component is fine (just library with no name, implicitly the name of the package). "Internal libraries" are named libraries that aren't exported, and apparently they don't work with Custom builds, but we shouldn't need one here.

@jmchapman
Copy link
Contributor Author

So, there's a difference between having an "internal library" and a library component at all. In particular, the "main" library component is fine (just library with no name, implicitly the name of the package). "Internal libraries" are named libraries that aren't exported, and apparently they don't work with Custom builds, but we shouldn't need one here.

Ah, thanks!

@jmchapman
Copy link
Contributor Author

jmchapman commented Aug 21, 2020

I don't think so... Have a look at the stanza for the plc exectuable in plutus-core: it uses modules from the main library, but doesn't have to repeat them all. It just declares a dependency on the library and then uses it like a normal package.

I think I have found where I was going wrong, the sources files for the lib and exe need to be in different folders otherwise cabal complains that the exe should repeat all the modules in other-modules. Perhaps it finds the files locally before looking for them in the library or something. This works for my test, I'll try moving stuff around in metatheory.

@michaelpj
Copy link
Contributor

Ah yes, the weird implicit module discovery that GHC does. Better to just put things in separate source folders indeed.

I moved tests into test/ and the executable shim into exe/
@jmchapman
Copy link
Contributor Author

I think I might need a bit of help/suggestions to update nix/haskell.nix. I have added a library and also renamed the three tests. I have tried to account for the latter but not the former.

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Did you want to rename the package too? My suggestion would be plutus-metatheory, since we're using the plutus- prefix for most of our packages.

metatheory/plc-agda.cabal Outdated Show resolved Hide resolved
metatheory/plc-agda.cabal Outdated Show resolved Hide resolved
nix/haskell.nix Outdated Show resolved Hide resolved
metatheory/plc-agda.cabal Outdated Show resolved Hide resolved
@jmchapman
Copy link
Contributor Author

Not sure why that's not working...

@michaelpj
Copy link
Contributor

Now it's just failing on the Haddock... tbh I'm not sure how that ever worked. I think I should maybe make the generation run as a postConfigure hook rather than preBuild 🤔

@michaelpj
Copy link
Contributor

Ah, there was no library so there was no Haddock!

@jmchapman jmchapman changed the title reorganizing plc-agda SCP-1066 SCP-1066 reorganizing plc-agda Aug 24, 2020
@jmchapman
Copy link
Contributor Author

Now it's just failing on the Haddock... tbh I'm not sure how that ever worked. I think I should maybe make the generation run as a postConfigure hook rather than preBuild 🤔

I can do that. Do you still think it might work?

@michaelpj
Copy link
Contributor

I'm just checking it out now.

@michaelpj
Copy link
Contributor

Okay, it's a bit awkward to do this because you're moving things, but just steal this commit: 155bee8

@jmchapman
Copy link
Contributor Author

Thanks @michaelpj that now passes CI. However, github seems to think that CI is still failing.

@michaelpj
Copy link
Contributor

Yep, it's just confused. Let's go ahead and merge it!

@michaelpj michaelpj marked this pull request as ready for review August 24, 2020 16:30
@michaelpj
Copy link
Contributor

Oh, it's caught up. Maybe turning it off from being a draft helped?

@michaelpj michaelpj merged commit 62178d9 into IntersectMBO:master Aug 24, 2020
@jmchapman
Copy link
Contributor Author

thanks!

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

Successfully merging this pull request may close these issues.

2 participants