dapp: package local remappings#719

Open

d-xo wants to merge 9 commits into

master from

package-local-remappings

Open

dapp: package local remappings #719
d-xo wants to merge 9 commits into
master from
package-local-remappings

Conversation

@d-xo

@d-xo d-xo commented Aug 2, 2021 •

edited

Loading

Copy link

Copy Markdown

Contributor

As discussed in #715 (comment) , this modifies dapp remappings so that it now generates a unique set of remappings for each src folder in the dependency tree that points into the adjacent lib folder.

This allows for multiple versions of a given package in the dependency tree, and means ds-pain is happily obsolete. You can check out https://github.com/dapphub/remappings-test for an example of a project that would be impossible to compile successfully with the old remappings shceme, but works fine with the new project local remappings.

The new remappings use the context field in the remappings format, which is supported back to at least solc 0.4.0 (I didn't check any further back...).

I'm still slightly on the fence if it's not better to just completely remove dapp remappings and force people to use relative paths for imports, but that would be a pretty major break to backwards compatibility, and I think probably ugly enough for larger projects to justify the continued existence of remappings for now.

There are a few breakages:

Projects will need to declare all their dependencies up front (e.g. if I dapp install ds-token, I only get ds-token available for import via remappings in the top level project, and must run a seperate dapp install ds-test to make ds-test available, even though ds-test is installed as a dep of ds-token). This means that some projects that would compile with an older version of dapp will fail with with newer versions. Maybe it's worth introducing some legacy compatibility options.
Since remappings are included in the metadata hash, this will break bit for bit bytecode verification when comparing the same project built with two different dapp versions. I don't see this as a big issue.

Are there considerations I'm missing? Was there a reason for enforcing the same version of a package across the dependcy tree?

Still needs changelogs and docs before merge...

@d-xo d-xo requested review from MrChico, livnev and rainbreak

August 2, 2021 11:01

@d-xo d-xo mentioned this pull request

Aug 2, 2021

Redo dependency management #715

Open

@MrChico

MrChico commented Aug 2, 2021

Copy link

Copy Markdown

Member

Exciting! I think this is a good approach to ameliorate the biggest pain point with a small change. Will investigate the consequences

@transmissions11

transmissions11 commented Aug 2, 2021

Copy link

Copy Markdown

Contributor

oo love this

@d-xo

d-xo commented Aug 4, 2021

Copy link

Copy Markdown

Contributor Author

As discussed in #715 (comment), solc can throw errors for contracts that make use of multiple inheritance if multiple versions of the same contract exist in the inheritance tree.

I therefore added a processing step to the remappings generation that deduplicates identical package versions, and remaps them all to the same path. I also added another test case to https://github.com/dapphub/remappings-test that demonstrates the issue (and compiles successfuly with the deduplication step). Note that the deduplication step will only help for inheritance trees where the multiple contract instances are all genuinly from exactly the same package version (i.e. identical git hash).

@d-xo d-xo force-pushed the package-local-remappings branch from d80f3c1 to d145258 Compare

August 4, 2021 16:03

@d-xo

d-xo commented Aug 4, 2021 •

edited

Loading

Copy link

Copy Markdown

Contributor Author

Added:

A legacy compatibility mode that allows direct imports from transitive dependencies if there is only a single version of that dependency in the tree
A fallback sha256sum based routine for getting a hash of a dependencies contents if the dep is not a git repo

With the above two changes we can build a complex project like tinlake-tests without needing to use custom remappings 🎉

For now the legacy mode is behind a flag instead of being the default since I think it's behaviour will be a little unpredictable (updating the version of a single dep deep in the dependency tree could break imports much higher up). Projects that need it can set it in a .dapprc or shell.nix file if required.

@d-xo

d-xo commented Aug 5, 2021

Copy link

Copy Markdown

Contributor Author

Added docs & changelog entry.

@d-xo

d-xo commented Aug 5, 2021

Copy link

Copy Markdown

Contributor Author

The biggest downside that I can see so far is that for complex projects the generated remappings can get quite huge. For example, if we run the new dapp remappings against tinlake-tests, we get this, whereas with the current approach, we get this (along with a ton of warnings related to mismatched package hashes....), which will make manually editing the generated remappings a lot harder (although hopefully this will not be needed nearly so much going forwards...).

@d-xo

d-xo commented Aug 5, 2021

Copy link

Copy Markdown

Contributor Author

Tested a bit with a few public repos, and it seems as though compatibility is generally good, with the exception of repos that depend on dss-deploy. This actually makes sense, since if I depend on dss-deploy, I probably also want to reuse the version of dss that it depends on. Starting to wonder if it doesn't make more sense to allow transitive imports by default 🤔

repo	default	--allow-transitive-imports
dss	✅	✅
tinlake-tests	❌	✅
drai	✅	✅
fuse-simulations	✅	✅
dss-cdp-manager	❌	✅
tinlake	✅	✅

@livnev livnev mentioned this pull request

Aug 11, 2021

dapp looks inside ./lib/ even for "innocent" commands #734

Open

@livnev

livnev commented Aug 11, 2021

Copy link

Copy Markdown

Member

I was told issue #734 might be relevant to this PR.

@d-xo d-xo mentioned this pull request

Aug 23, 2021

Added hevm.sol dapphub/ds-test#21

Open

@d-xo d-xo force-pushed the package-local-remappings branch from 2e25146 to fa9fd30 Compare

September 7, 2021 11:03

MrChico

MrChico reviewed

Sep 8, 2021

View reviewed changes

src/dapp/libexec/dapp/dapp

--extract) export DAPP_BUILD_EXTRACT=1;;

--optimize) export DAPP_BUILD_OPTIMIZE=1;;

--legacy) export DAPP_BUILD_LEGACY=1;;

--allow-transitive-imports) export DAPP_ALLOW_TRANSITIVE_IMPORTS=1;;

@MrChico MrChico Sep 8, 2021

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

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

Can we have a shorter alias? --trans?

@transmissions11 transmissions11 Sep 8, 2021

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Can we have a shorter alias? --trans?

ayy my own flag

MrChico

MrChico approved these changes

Sep 8, 2021

View reviewed changes

@MrChico

MrChico commented Sep 8, 2021

Copy link

Copy Markdown

Member

lfg

@d-xo d-xo force-pushed the package-local-remappings branch from fa9fd30 to 070ecc4 Compare

September 8, 2021 14:24

d-xo added 9 commits

September 8, 2021 18:07

@d-xo


 package local remappings

2dcf5fc

This modifies dapp remappings so that it now generates a unique set of
remappings for each `src` folder in the dependency tree that points into
the adjacent `lib` folder.
This allows for multiple versions of a given package in the dependency
tree, and means `ds-pain` is happily obsolete.

@d-xo


 dapp: remappings: deduplicate identical packages

c2613b1

@d-xo


 dapp: remappings: minor refactoring

1cce6cd

@d-xo


 dapp: remappings: allow non git folders in lib dir

b34a105

@d-xo


 dapp: remappings: add flag for legacy compat

90ca996

@d-xo


 dapp: remappings: document new format, rename & expose legacy compat ...

aa5302d

...flag

@d-xo


 dapp: remappings: fix transitive-imports flag

c01b0b5

@d-xo


 dapp: remappings: use shortest path when deduplicating

e42da31

@d-xo


 dapp: remappings: normalize DAPP_SRC and DAPP_LIB

b9e5d92

@d-xo d-xo force-pushed the package-local-remappings branch from 070ecc4 to b9e5d92 Compare

September 8, 2021 16:08

@d-xo

d-xo commented Sep 9, 2021

Copy link

Copy Markdown

Contributor Author

After discussing with @rainbreak over the last few days I think I'm gonna let this sit a little longer and experiment with some alternative approaches. In particular it may be better to make a full break and jump straight to a workflow involving a fully flattened directory hierarchy under lib, which would significantly reduce network and disk bloat, while also making workflows that involve local edits to dependencies much more predictable.

Labels

None yet

4 participants

@d-xo @MrChico @transmissions11 @livnev

Conversation

@d-xo d-xo commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrChico commented Aug 2, 2021

Uh oh!

transmissions11 commented Aug 2, 2021

Uh oh!

d-xo commented Aug 4, 2021

Uh oh!

d-xo commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-xo commented Aug 5, 2021

Uh oh!

d-xo commented Aug 5, 2021

Uh oh!

d-xo commented Aug 5, 2021

Uh oh!

livnev commented Aug 11, 2021

Uh oh!

@MrChico MrChico Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

@transmissions11 transmissions11 Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

MrChico commented Sep 8, 2021

Uh oh!

d-xo commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@d-xo d-xo commented Aug 2, 2021 •

edited

Loading

d-xo commented Aug 4, 2021 •

edited

Loading