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

Merged
merged 5 commits into from
Dec 27, 2024

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
@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added them in 52b257b

gas1cent
gas1cent previously approved these changes Dec 13, 2024
const matches = [...iniAsStr.matchAll(/(?:path|url) = (.*)/g)].map(regexMatch => regexMatch[1]);
if (matches.length == 0) return {};

// this assumes .gitmodules to be well formed, same amount of paths and urls
Copy link
Contributor

Choose a reason for hiding this comment

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

If is not, shouldn't it throw an error describing the issue?
I see this as a NTH tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

52b257b
added a test for a valid .gitmodules that breaks it, but am not explicitly throwing an error in that case, as figuring out if the precondition is met or not would involve extra logic

0xDiscotech
0xDiscotech previously approved these changes Dec 13, 2024
Copy link
Contributor

@0xDiscotech 0xDiscotech left a comment

Choose a reason for hiding this comment

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

Approved with a comment, agreed with Tony that would be nice to cover the case where the submodules file contains a different format than what we are expecting.

Nice one!

@0xteddybear 0xteddybear dismissed stale reviews from 0xDiscotech and gas1cent via 52b257b December 16, 2024 17:21
@gas1cent
Copy link
Member

I've tried running the tests locally and got the following output. Looks like the file mock doesn't work, because creating the remappings.txt file manually results in the output containing different errors. I'm on a mac, using node v22 if that helps.

1) parseGitmodulesDependencies
      with a file with several dependencies
        should be able to parse them:
    AssertionError: expected undefined to equal 'https://github.com/foundry-rs/forge-s…'
    at Context.<anonymous> (test/unit/parseGitmodulesDependencies.spec.ts:47:50)
    at processImmediate (node:internal/timers:491:21)

2) parseGitmodulesDependencies
      with a file with extra (valid) keys besides path and url
        should be able to parse them:
    AssertionError: expected undefined to equal 'https://github.com/foundry-rs/forge-s…'
    at Context.<anonymous> (test/unit/parseGitmodulesDependencies.spec.ts:72:50)
    at processImmediate (node:internal/timers:491:21)

3) transformRemappings
      should leave unrelated imports alone:
    AssertionError: expected '' to include 'import \'./Contract.sol\';'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:161:35)
    at processImmediate (node:internal/timers:491:21)

4) transformRemappings
      should leave unrelated statements alone:
    AssertionError: expected '' to include '// import \'./Contract.sol\';'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:178:35)
    at processImmediate (node:internal/timers:491:21)

5) transformRemappings
      with remapping for local file only
        should replace remapped path if its not at the beggining of string:
    AssertionError: expected '' to include 'import \'../../../../interfaces/ITest…'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:29:37)
    at processImmediate (node:internal/timers:491:21)

6) transformRemappings
      with remapping for local file only
        should replace path as instructed by remapping with all import syntaxes:
    AssertionError: expected '' to include 'import \'../../interfaces/ITest.sol\';'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:48:37)
    at processImmediate (node:internal/timers:491:21)

7) transformRemappings
      with remapping for local file only
        should remove node_modules from import paths:
    AssertionError: expected '' to include 'import \'some-package/Contract.sol\';'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:72:37)
    at processImmediate (node:internal/timers:491:21)

8) transformRemappings
      with remapping for path inside node_modules
        should apply remapping and then remove node_modules from path:
    AssertionError: expected '' to include 'import {FixedPointMathLib} from \'sol…'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:106:37)
    at processImmediate (node:internal/timers:491:21)

9) transformRemappings
      with remapping for path inside lib/
        should apply remapping and then remove lib from path:
    AssertionError: expected '' to include 'import {FixedPointMathLib} from \'sol…'
    at Context.<anonymous> (test/unit/transformRemappings.spec.ts:137:37)
    at processImmediate (node:internal/timers:491:21)

`,
});
});
it.skip('should be able to parse them', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Why are some tests being skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to acknowledge, yet thoroughly document the limitations of the current implementation. Example of a previous discussion: #58 (comment)

This test in particular was added to document the consequences of not doing proper git-config format parsing (I couldn't find an existing implementation, and the generic .ini parser in npm was not enough either)

@0xteddybear
Copy link
Contributor Author

0xteddybear commented Dec 24, 2024

@gas1cent

using node v22

oh, sorry I didn't add it in the description, the file mocking lib works only on node versions <=18 , that is actually the blocker for #57 , which I will get to at a later time 😬

Copy link
Contributor

@0xDiscotech 0xDiscotech left a comment

Choose a reason for hiding this comment

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

Tests passed using 18 as node version. Nicely done ser!

@0xteddybear 0xteddybear merged commit b02e984 into main Dec 27, 2024
3 checks passed
@0xteddybear 0xteddybear deleted the feat/gitmodules-dependencies branch December 27, 2024 14:35
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.

4 participants