-
Notifications
You must be signed in to change notification settings - Fork 765
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
devp2p monorepo transition #971
Conversation
#5 : Remove Peer on close event
Reactivate library
…usage and events description
Improve documentation
Ok, CI is passing. 😄 Ready for review, I would ask you to not demand further rebases on this since this is delicate with these merged Git histories, not worth for consolidating on a few extra commits. |
I checked the commits which you built upon the current master branch of the package, which look good to me. However, git history does not seem to be viewed correctly. Maybe this has to do with it? |
@jochem-brouwer github has some issues showing history across renames, see isaacs/github#900 |
Wow it seems like GitHub should add a high priority to this feature. I don't understand how this is not a thing yet! I recall that I ran into this problem as well to track the |
yeah it's a bummer 😕 i'm also surprised they haven't allocated more resources toward it, i would find it useful multiple times per week, especially with the monorepo and also all our js=>ts file renamings. gitlab shows it properly...ev imported the monorepo in gitlab once to test that his script modified the git history correctly and it looked nice. |
Any chance that this still gets an approval tonight, or do you guys have further reservations? Then I could directly tackle the client transition tomorrow as some follow-up. |
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 LGTM!
@ryanio thanks 😄 , @jochem-brouwer (or @cgewecke) please give this an additional approval when you are confident here as well. |
Ah yes sorry I wanted to check this, this afternoon.
Output
This should show the commit history, right? EDIT: this was called on a non-existent directory in the original repo so the results make sense, if you call this on any file it works just fine. |
If I checkout this branch and use simple For example, somewhere below the ORIGIN/HEAD master commit, I find this, (part of this PR). Local
Are you expecting (or seeing) something else? |
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.
LGTM 🎉
Great job, @holgerd77! @jochem-brouwer yes, the I didn't check, but please make sure the tests for devp2p are present on the newly added node-versions CI file. |
Also, if you want to update the diagram, you can click on the URL at line 35 of the README.md (Mermaid chart). Then, add the package reference to devp2p, copy the SVG code and the new bug URL into the README. |
One note here: when I wanted to pull this into my local |
@cgewecke yes the commits are in the commit log, but if I try to track where these devp2p files originated from by |
Yes you are right @holgerd77, I called this on a directory which did not exist in the original |
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.
Approved ✅
This PR transitions the ethereumjs-devp2p library to the monorepo, partly addresses #892
Ok, this should hopefully do it, transition is inspired by the monorepify script from Everton, I used a simple version though and extracted the commands useful for this transition and for the main part followed the instructions from this post linked in the script.
Git history is preserved, I tested a merge on my own fork here https://github.com/holgerd77/ethereumjs-vm. Commit history looks like things are put into proper order, see also e.g. here for an example commit diff. This should be the main thing, if there are smaller caveats - some CI, dependency, link stuff to fix - we can also do post-merge.
Also updated the following:
package.json
for hoistingREADME
linksREADME
sectionsPlease nevertheless do not merge directly, I would do tomorrow (CET) in case of successful reviews. I would wait for 2+ reviews on this before merge.