-
Notifications
You must be signed in to change notification settings - Fork 482
Vendor the flat package
#7308
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
Vendor the flat package
#7308
Conversation
|
flat package
kwxm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks OK, and it appears to work as it should. I'm not familiar with licensing issues and the details of how one deals with incorporating someone else's code into the codebase though, so we should probabaly get someone else to review it too.
cabal.project
Outdated
| plutus-benchmark | ||
| plutus-conformance | ||
| plutus-core | ||
| plutus-core/flat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that references to flat in the .cabal files will get our version now? I'm not sure how things get resolved if there are multiple libraries with the same name floating about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Cabal first resolves flat via cabal.project's packages and S.R.P stanzas and if that fails it looks into hackage and CHaP
| @@ -0,0 +1,30 @@ | |||
| Copyright (c) 2016, Pasqualino `Titto` Assini | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should remove this license if we're going to start modifying the code. Maybe there's some company policy about this kind of situation? I see that the license does say that we're supposed to retain it, but what happens if we change the source code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license says that ridistribution is permitted so long as the original LICENSE file is retained, AFAIU.
But we should get a second opinion.
plutus-core/flat/flat.cabal
Outdated
| @@ -0,0 +1,171 @@ | |||
| cabal-version: 3.0 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flat code is independent of the rest of the Plutus code at the moment. I wonder if we should put it in a separate package, like plutus-flat or something. I don't know if that would be sensible or not, but it seems like the sort of thing that people mighr have an opinion about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A top-level plutus-flat folder is just as good, probably even a better idea.
We should vote: who's in favour of a top-level plutus-flat as opposed to plutus-core/flat? React with 👍 @IntersectMBO/plutus-core-maintainers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without having a strong preference I'd chose plutus-core/flat since we already keep other libs like satint there and plutus-core package serves as a namespace. But top level isn't bad too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the other libs like satint in plutus-core instead of separate packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top-level with its own .cabal file is better - so that it is just a mirror of the original project's repository. Easier to upstream changes this way.
Why are the other libs like satint in plutus-core instead of separate packages?
Is satint a vendored library? Seems not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I lean slightly towards putting it at the top level because it'd be a bit easier to work on, but it's not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top-level with its own .cabal file is better - so that it is just a mirror of the original project's repository. Easier to upstream changes this way.
I agree with this.
92b9b25 to
51d2845
Compare
plutus-core/plutus-core.cabal
Outdated
| , extra | ||
| , filepath | ||
| , flat ^>=0.6 | ||
| , flat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plutus-core:flat?
Vendor the flat library in plutus:
benchmarksfolderDocTesttest-suitecabal-version: 3.0inflat.cabaldefault-extensions: ImportQualifiedPosttoflat.cabaland appliedstylish-haskellimport Data.ByteString.Internal.memcpywithimport Foreign.Marshal.Utils (copyBytes)as per deprecation errors.PlutusCore.Flatmodule in plutus-core has been renamed toPlutusCore.FlatInstances