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

Build/npm ignore file #936

Merged
merged 4 commits into from
Jun 14, 2024
Merged

Build/npm ignore file #936

merged 4 commits into from
Jun 14, 2024

Conversation

andreivladbrg
Copy link
Member

@PaulRBerg I've also included your commit

Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

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

LGTM.

@PaulRBerg
Copy link
Member

Thanks @andreivladbrg, just FYI in the future, you can also add me as a co-author.

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

Let's not merge this as is.

we should not use a .npmignore file because it adds very complicated logic when files are used at the same time.

I am no longer able to find the reference for this but I left a comment in a Medium post about this issue many years ago.

edit: found it:

@andreivladbrg
Copy link
Member Author

just FYI in the future, you can also add me as a co-author.

which commit are you referring to? i used git cherry-pick and you are the author of the commit:

image

we should not use a .npmignore file because it adds very complicated logic when files are used at the same time.

sheesh, didn't know about this

Let's not merge this as is.

do you have any suggestion?

@PaulRBerg
Copy link
Member

PaulRBerg commented May 29, 2024

All I meant is that instead of cherrypicking, you could have added me as a co-author. Judt FYI. Cherrypicking was fine in this case.

re sheesh - just use files, as I have suggested initially. You can put negative globs like this:

{
  "files": ["!test/utils/*.t.sol"]
}

Also, why not ignore all *.t.sol files? not just those in test/utils.

@andreivladbrg
Copy link
Member Author

andreivladbrg commented May 29, 2024

just use files, as I have suggested initially. You can put negative globs like this:

@PaulRBerg I've added this in package.json but it doesn't seem to work, as seen here:

https://app.warp.dev/block/CRhLQnKpcZGWmIdLnGpveL

what if we include this CLI in the prepack:

"find . -name 'test/utils/*.t.sol' -delete",

and after we can git restore

@PaulRBerg
Copy link
Member

The negative globs should definitely work - make sure to put it at the end of the array in files. And ask ChatGPT for help. It must be possible.

@PaulRBerg
Copy link
Member

It appears that I was wrong - negative globs are not supported in the files field.

Therefore, it's impossible to ignore the *.t.sol files without using an .npmignore file.

I pushed a commit to add an .npmignore file in the test directory. It has to be added there because otherwise npm will ignore the ignore file itself.

It should be working now - please double-check before merging @andreivladbrg @smol-ninja. Then, squash and merge, please.

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

lgtm now

andreivladbrg and others added 4 commits June 14, 2024 15:14
build: exclude *.t.sol files from NPM package
build: add "!test/utils/*.t.sol" in package json files
@smol-ninja
Copy link
Member

Thanks @PaulRBerg for the fix. It seems to be working now: https://app.warp.dev/block/8z3gVbenfdyKFq3RsDoFKj

@andreivladbrg
Copy link
Member Author

great, ty to both🚀

@andreivladbrg andreivladbrg merged commit c223021 into staging Jun 14, 2024
8 of 9 checks passed
@andreivladbrg andreivladbrg deleted the build/npm-ignore-file branch June 14, 2024 12:48
andreivladbrg added a commit that referenced this pull request Jul 3, 2024
* build: add npm ignore file

build: exclude *.t.sol files from NPM package

* test: absolute paths in precompiles test

* build: remove .npmignore

build: add "!test/utils/*.t.sol" in package json files

* build: npm ignore file in test utils

---------

Co-authored-by: Paul Razvan Berg <prberg@proton.me>
andreivladbrg added a commit that referenced this pull request Jul 3, 2024
* build: add npm ignore file

build: exclude *.t.sol files from NPM package

* test: absolute paths in precompiles test

* build: remove .npmignore

build: add "!test/utils/*.t.sol" in package json files

* build: npm ignore file in test utils

---------

Co-authored-by: Paul Razvan Berg <prberg@proton.me>
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