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

feat: barebones dependency conversion #61

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0xteddybear
Copy link
Contributor

@0xteddybear 0xteddybear commented Nov 19, 2024

merge after #58

closes #60

adding support for git dependencies means we'll probably also support projects where the dependencies are not explicitly stated in remappings.txt, the build instead relying on foundry's implicit remappings. To fully handle those cases, this project should invoke forge remappings instead of reading remappings.txt

TODO

  • convert dependencies
    • do so robustly -- se gitmodules parsing below
  • replace lib similarly to node_modules in transformRemappings

.gitmodules parsing

I tried to use the ini npm package since .gitmodules seem to be .ini files, but didn't behave as expected.

It didn't recognize the various path and url fields as part of the relevant submodule subsections, instead assignig them as items of submodule, so after parsing an entire file only the last two remained:

[submodule foo]
  path=foo
  url=http://foo
[submodule bar]
  path=bar
  url=http://bar

would yield

{
  path: bar,
  url: http://bar
}

The current implementation works for machine-generated .gitmodules, but will break if a subseciton is populated in two passes like so:

[submodule foo]
  path=foo
[submodule bar]
  path=bar
  url=http://bar
[submodule foo]
  url=http://foo

or if path and url are out of order (which is perfectly valid for git)

[submodule foo]
  url=http://foo
  path=foo

and probably other cases that are syntactically valid ini, and sematically valid submodule definitions, but that dont match my arbitrary regex

I deem this good enough for now, specially considering we are doing the same problematic regex-parsing for solidity files

@0xteddybear 0xteddybear force-pushed the feat/gitmodules-dependencies branch 2 times, most recently from c2d625a to 7c818ed Compare November 20, 2024 14:17
@0xteddybear 0xteddybear marked this pull request as ready for review November 20, 2024 15:59
describe('with no gitmodules file', function () {
before(() => {
mock({
'.gitmodules': '',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the mock here? I suspect it will make it look like the file exists and is empty, and ideally we should test the case of the file not being present at all, in particular to ensure that the read call doesn't revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good find 🤠 fixed it in cd631e6

@0xteddybear 0xteddybear force-pushed the feat/gitmodules-dependencies branch from 5f90b8c to 6993ff6 Compare November 26, 2024 21:18
Base automatically changed from fix/double-import to main November 27, 2024 20:50
@0xteddybear 0xteddybear force-pushed the feat/gitmodules-dependencies branch from cd631e6 to c4b2b29 Compare November 27, 2024 21:07
@0xteddybear 0xteddybear requested a review from gas1cent November 27, 2024 21:10
Copy link
Member

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

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

LGTM!

expect(parsedDependencies).to.deep.eq({});
});
});
describe('with a file with several dependencies', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth also having a case where the gitmodules exist but is not in the format we expect it

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.

3 participants