search

pulsar-edit / superstring

Native core components for Pulsar
MIT License
1 stars 5 forks source link

Vendor `win-iconv` as regular files, not as a submodule (+ fix CI on Windows) #6

Closed DeeDeeG closed 11 months ago

DeeDeeG commented 12 months ago

Add win-iconv files using git subtree

This replaces what used to be a submodule of the win-iconv repo.

Using git subtree preserves revision history, author info (including git blame view), and so on.

win-iconv is a very small repo, it's public domain, and it has few revisions. So, I think it's worth it to do all that, for giving proper attribution of author info, if for no other reason.

Not using a submodule is more compatible with newer npm (npm 7.x+), and apparently with Yarn (v1.x?) as well. It's overall less complicated for end users of the superstring package/repo.

Add 'vendor/win-iconv/' from commit ' 9f98392dfecadffd62572e73e9aba878e03496c4 '

git-subtree-dir: vendor/win-iconv git-subtree-mainline: 5b2f39d3319ab9a568b1afff4a23be9ece458cc1 git-subtree-split: 9f98392dfecadffd62572e73e9aba878e03496c4

DeeDeeG commented 12 months ago

NERDY GIT NOTES (or why this PR shows so many commits in it):

The alternative to using git subtree would be to just delete the submodule (as this PR does) and plop the plain files down at vendor/win-iconv, not set it up there with git subtree.

The downside of simply plopping the files down is it makes me appear to be the author of the files for the purposes of git blame. But it would be a lighter-weight way of doing it, since the win-iconv commit history would not be added to this git repo.

I'm surprised how much I show as the contributor for vendor/win-iconv in the GitHub UI and in git log, though. So, this maybe didn't accomplish as much transparency about the authorship of the files as I had hoped. I doubt we will be git blameing in the vendor/win-iconv files in this repo very often, for files that haven't changed since 2016??? But if that ever happens, the way this PR is currently doing things is the vastly superior choice. I just have no idea if using git blame like that will ever come up at all.

If having this many commits added in this PR is too annoying, confusing or distracting, and if we're never going to git blame these vendor/win-iconv files, then vendoring them as plain files (instead of using git subtree) may be preferable.

DeeDeeG commented 12 months ago

The CI failure (Node 14 on Windows) is because of old node-gyp. Node 14 is old, basically. The latest Node v14 was Node v14.21.3, which came with npm v6.14.18, which came with node-gyp v5.1.1.

Node-gyp v5.1.1 doesn't support Visual Studio 2022, which is the default in the windows-latest GitHub-hosted runner now.

I'm going to push a change to the test matrix and see if I can make it work.

DeeDeeG commented 12 months ago

A weird intermittent (one-time only???) CI failure again... One of the macOS jobs failed because of the invalid mode 'rU' thing, but node-gyp v5.1.1 should be using Python2 preferentially, I don't even know why the heck it would hit a Python 3.11-related error?????????

I re-ran it and it's fine??????????????????????????????????????????????????????/

(Please ignore the amount of question marks, I am going insane, why you gotta do me like this, CI?)

This is fine. (Click to expand): ![this-is-fine-meme-dog-sitting-in-a-firey-house-with-a-coffee-mug-saying-this-is-fine](https://cdn.vox-cdn.com/thumbor/1YUH-cgyaRITZBMtRqt0bRMFuOM=/40x0:541x282/1600x900/cdn.vox-cdn.com/uploads/chorus_image/image/50231371/post-64231-this-is-fine-dog-fire-comic-Im-N7mp.0.png)
DeeDeeG commented 11 months ago

You can verify this is done properly by doing a diff of the before and after.

My steps to test (on macOS) and results (click to expand): ```console % git clone --recursive https://github.com/pulsar-edit/superstring Cloning into 'superstring'... remote: Enumerating objects: 7058, done. remote: Counting objects: 100% (299/299), done. remote: Compressing objects: 100% (132/132), done. remote: Total 7058 (delta 169), reused 270 (delta 160), pack-reused 6759 Receiving objects: 100% (7058/7058), 2.09 MiB | 2.63 MiB/s, done. Resolving deltas: 100% (4498/4498), done. Submodule 'vendor/win-iconv' (https://github.com/win-iconv/win-iconv) registered for path 'vendor/win-iconv' Cloning into '/Users/User/superstring-diff-test/superstring/vendor/win-iconv'... remote: Enumerating objects: 187, done. remote: Total 187 (delta 0), reused 0 (delta 0), pack-reused 187 Receiving objects: 100% (187/187), 53.76 KiB | 1.28 MiB/s, done. Resolving deltas: 100% (114/114), done. Submodule path 'vendor/win-iconv': checked out '9f98392dfecadffd62572e73e9aba878e03496c4' % cd superstring % cp -r vendor/win-iconv ./_win-iconv_old % rm -r vendor/win-iconv # Must delete these files, or there will be a conflict checking out the other branch % git switch vendor-win-iconv-as-regular-files-not-a-submodule branch 'vendor-win-iconv-as-regular-files-not-a-submodule' set up to track 'origin/vendor-win-iconv-as-regular-files-not-a-submodule'. Switched to a new branch 'vendor-win-iconv-as-regular-files-not-a-submodule' % diff --report-identical-files ./_win-iconv_old vendor/win-iconv Only in ./_win-iconv_old: .git Files ./_win-iconv_old/CMakeLists.txt and vendor/win-iconv/CMakeLists.txt are identical Files ./_win-iconv_old/ChangeLog and vendor/win-iconv/ChangeLog are identical Files ./_win-iconv_old/FindWcecompat.cmake and vendor/win-iconv/FindWcecompat.cmake are identical Files ./_win-iconv_old/Makefile and vendor/win-iconv/Makefile are identical Files ./_win-iconv_old/iconv.def and vendor/win-iconv/iconv.def are identical Files ./_win-iconv_old/iconv.h and vendor/win-iconv/iconv.h are identical Files ./_win-iconv_old/mlang.def and vendor/win-iconv/mlang.def are identical Files ./_win-iconv_old/mlang.h and vendor/win-iconv/mlang.h are identical Files ./_win-iconv_old/readme.txt and vendor/win-iconv/readme.txt are identical Files ./_win-iconv_old/win_iconv.c and vendor/win-iconv/win_iconv.c are identical Files ./_win-iconv_old/win_iconv_test.c and vendor/win-iconv/win_iconv_test.c are identical ```
Just the diff results (click to expand): ```console % diff --report-identical-files ./_win-iconv_old vendor/win-iconv Only in ./_win-iconv_old: .git Files ./_win-iconv_old/CMakeLists.txt and vendor/win-iconv/CMakeLists.txt are identical Files ./_win-iconv_old/ChangeLog and vendor/win-iconv/ChangeLog are identical Files ./_win-iconv_old/FindWcecompat.cmake and vendor/win-iconv/FindWcecompat.cmake are identical Files ./_win-iconv_old/Makefile and vendor/win-iconv/Makefile are identical Files ./_win-iconv_old/iconv.def and vendor/win-iconv/iconv.def are identical Files ./_win-iconv_old/iconv.h and vendor/win-iconv/iconv.h are identical Files ./_win-iconv_old/mlang.def and vendor/win-iconv/mlang.def are identical Files ./_win-iconv_old/mlang.h and vendor/win-iconv/mlang.h are identical Files ./_win-iconv_old/readme.txt and vendor/win-iconv/readme.txt are identical Files ./_win-iconv_old/win_iconv.c and vendor/win-iconv/win_iconv.c are identical Files ./_win-iconv_old/win_iconv_test.c and vendor/win-iconv/win_iconv_test.c are identical ```

This output is good. It shows that all the actual files of the win-iconv repo are checked in on this PR's branch identically to how they were in the submodule. But now that the vendor/win-iconv/ dir isn't a submodule anymore, it doesn't have (or need) its own git repo metadata dir vendor/win-iconv/.git/ anymore. This is all good, and the diff command output matches exactly with what was intended.

DeeDeeG commented 11 months ago

Thanks for the approve, merging now!