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

Enable support for gawk in-place editing #232

Merged
merged 1 commit into from
Mar 2, 2019
Merged

Enable support for gawk in-place editing #232

merged 1 commit into from
Mar 2, 2019

Conversation

landstander668
Copy link
Contributor

@landstander668 landstander668 commented Mar 1, 2019

Update the excluded file patterns, so the contents of "/usr/share/awk" will be present in the installer and portable builds. This does not significantly impact the size of the installer, with the full contents of the directory being approximately 50 KB in size.

This fixes git-for-windows/git#1972.

@landstander668
Copy link
Contributor Author

I can confirm that after upgrading gawk to version 4.2.1-2 via pacman -S msys/gawk, the /usr/lib/gawk/inplace.dll dynamic library is present (the old version had only the static libraries), and the example command from issue #1972 now works successfully.

@dscho Is there a special process for updating the MSYS2 packages? For example, do we just redistribute the upstream package (assuming this one wasn't customized specifically for Git for Windows)?

If it needs to be rebuilt, I imagine that's basically just pulling the upstream metadata into MSYS2-packages/gawk, building the package, and verifying that it fixes the issue in question. Which would then simply be turned into a separate PR against MSYS2-packages repository. Does this sound correct?

@dscho
Copy link
Member

dscho commented Mar 1, 2019

Is there a special process for updating the MSYS2 packages?

No, we simply consume most of the MSYS2 packages provided by the MSYS2 project; we do ship our own versions of a couple of them, though, such as OpenSSH (because I really do not want to wait for, or lean on, the good MSYS2 people to pick up critical security updates).

You can verify which package version is used in any given Git for Windows version by looking at the versions/ subdirectory in the build-extra repository, for v2.21.0 it was gawk 4.2.1-1, for example.

And you can see what the current version is in Git for Windows' SDK by looking at https://github.com/git-for-windows/git-sdk-64/tree/master/var/lib/pacman/local; apparently, we override gawk with our own version for some reason.

@dscho
Copy link
Member

dscho commented Mar 1, 2019

we override gawk with our own version for some reason.

I guess the reason is git-for-windows/MSYS2-packages@e65e181

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for picking this up.

In addition to the simplification I suggested in the diff, I'd also like for a change of the commit message: instead of saying "Its omission was reported as item #1972 in the issue tracker." could you say "This fixes git-for-windows/git#1972" at the end of the commit message? That will auto-close that ticket when I merge the PR.

Thanks!

@@ -156,6 +156,7 @@ grep -v -e '\.[acho]$' -e '\.l[ao]$' -e '/aclocal/' \
-e '^/usr/share/perl5/core_perl/TAP/' \
-e '^/usr/share/vim/vim74/lang/' \
-e '^/etc/profile.d/git-sdk.sh$' |
grep -v -P '^/usr/share/awk(?!/inplace\.awk$)' |
Copy link
Member

Choose a reason for hiding this comment

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

How about dropping this?

$ du -hs /usr/share/awk/
54K     /usr/share/awk/

That's not really anything to worry over, it will be compressed well by LZMA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. :) I'll update the commit to address both items this evening, and force-push the PR branch.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@landstander668
Copy link
Contributor Author

As for the upstream gawk package, I'll try to verify whether or not it already incorporates (or otherwise addresses) git-for-windows/MSYS2-packages@e65e181 and get back to you.

@landstander668
Copy link
Contributor Author

@dscho It looks to me like your fix from git-for-windows/MSYS2-packages@e65e181 has been effectively incorporated upstream. The only difference I see are:

  • Their version of PKGBUILD uses the patch -p1 -i filename syntax instead of patch -p1 <filename.
  • Their version of fix-cr.patch uses #ifdef __MSYS__ in place of #ifdef __CYGWIN__.

Unless I'm missing something, neither change is functionally different. So it seems like the best course of action is to simply go back to consuming the upstream gawk package.

This presumably this means that we need a SDK update, but I'm not sure if that's enough by itself to pull the new version into the build process. Any suggestions?

@dscho
Copy link
Member

dscho commented Mar 1, 2019

This presumably this means that we need a SDK update

How about leaving gawk as-is? AFAICT going back to consuming the upstream MSYS2 package would not change anything, but add more work for me. So I'd rather not, unless there is a good reason to do that...

@landstander668
Copy link
Contributor Author

The one thing it would do—in conjunction with this PR—is allow the awk in-place editing to work. But if you believe this takes things too far outside the Git for Windows scope, I'm happy to go along with your assessment.

@dscho
Copy link
Member

dscho commented Mar 1, 2019

The one thing it would do—in conjunction with this PR—is allow the awk in-place editing to work

Why? Our current gawk package has the inplace.awk file...

@dscho
Copy link
Member

dscho commented Mar 1, 2019

@landstander668
Copy link
Contributor Author

That's true, but our current version does not have the supporting shared library in /usr/lib/gawk. It only includes the static libraries, which can't be loaded at runtime. This was an upstream issue with the gawk 4.2.1-1 package, which presumably just carried over into the Git for Windows counterpart, that's fixed in 4.2.1-2.

Here's what /usr/lib/gawk from the SDK currently looks like.

$ ls -l /usr/lib/gawk
total 104
-rw-r----- 1 adric 1049089 26968 Feb 28 08:24 libfilefuncs.a
-rw-r----- 1 adric 1049089  4468 Feb 28 08:24 libfnmatch.a
-rw-r----- 1 adric 1049089  3760 Feb 28 08:24 libfork.a
-rw-r----- 1 adric 1049089  8234 Feb 28 08:24 libinplace.a
-rw-r----- 1 adric 1049089  5692 Feb 28 08:24 libintdiv.a
-rw-r----- 1 adric 1049089  2908 Feb 28 08:24 libordchr.a
-rw-r----- 1 adric 1049089  4230 Feb 28 08:24 libreaddir.a
-rw-r----- 1 adric 1049089  4430 Feb 28 08:24 libreadfile.a
-rw-r----- 1 adric 1049089  3092 Feb 28 08:24 librevoutput.a
-rw-r----- 1 adric 1049089  4412 Feb 28 08:24 librevtwoway.a
-rw-r----- 1 adric 1049089  7472 Feb 28 08:24 librwarray.a
-rw-r----- 1 adric 1049089  2992 Feb 28 08:24 libtime.a

After installing the upstream gawk 4.2.1-2, the directory now contains:

$ ls -l /usr/lib/gawk
total 224
-rwxr-x--- 1 adric 1049089 26614 Dec  9 05:51 filefuncs.dll
-rw-r----- 1 adric 1049089  8798 Dec  9 05:51 filefuncs.dll.a
-rwxr-x--- 1 adric 1049089 11238 Dec  9 05:51 fnmatch.dll
-rw-r----- 1 adric 1049089  3340 Dec  9 05:51 fnmatch.dll.a
-rwxr-x--- 1 adric 1049089 10367 Dec  9 05:51 fork.dll
-rw-r----- 1 adric 1049089  3316 Dec  9 05:51 fork.dll.a
-rwxr-x--- 1 adric 1049089 14226 Dec  9 05:51 inplace.dll
-rw-r----- 1 adric 1049089  3340 Dec  9 05:51 inplace.dll.a
-rwxr-x--- 1 adric 1049089 12067 Dec  9 05:51 intdiv.dll
-rw-r----- 1 adric 1049089  3334 Dec  9 05:51 intdiv.dll.a
-rwxr-x--- 1 adric 1049089  9608 Dec  9 05:51 ordchr.dll
-rw-r----- 1 adric 1049089  3334 Dec  9 05:51 ordchr.dll.a
-rwxr-x--- 1 adric 1049089 10316 Dec  9 05:51 readdir.dll
-rw-r----- 1 adric 1049089  3340 Dec  9 05:51 readdir.dll.a
-rwxr-x--- 1 adric 1049089 10781 Dec  9 05:51 readfile.dll
-rw-r----- 1 adric 1049089  3356 Dec  9 05:51 readfile.dll.a
-rwxr-x--- 1 adric 1049089  9637 Dec  9 05:51 revoutput.dll
-rw-r----- 1 adric 1049089  3362 Dec  9 05:51 revoutput.dll.a
-rwxr-x--- 1 adric 1049089 10550 Dec  9 05:51 revtwoway.dll
-rw-r----- 1 adric 1049089  3362 Dec  9 05:51 revtwoway.dll.a
-rwxr-x--- 1 adric 1049089 13377 Dec  9 05:51 rwarray.dll
-rw-r----- 1 adric 1049089  3340 Dec  9 05:51 rwarray.dll.a
-rwxr-x--- 1 adric 1049089  9711 Dec  9 05:51 time.dll
-rw-r----- 1 adric 1049089  3316 Dec  9 05:51 time.dll.a

Without the shared inplace.dll file, the sample command from git-for-windows/git#1972 fails with the exact same error as in the current Git for Windows 2.21.0 release, regardless of whether or not inplace.awk is included. It also doesn't work in the SDK—where /usr/share/awk/inplace.awk has been present from the beginning—unless the upstream gawk upgrade is installed.

So we can't really fix the issue without including both pieces. Unless, naturally, this is decided to be outside the scope of Git for Windows, and therefore not worth addressing.

@dscho
Copy link
Member

dscho commented Mar 1, 2019

Without the shared inplace.dll file, the sample command from git-for-windows/git#1972 fails with the exact same error as in the current Git for Windows 2.21.0 release

Okay, that's the piece I was missing.

@dscho
Copy link
Member

dscho commented Mar 1, 2019

FYI I decided to simply re-build gawk v4.2.1-2, as I do not have any automation for "unshadowing" MSYS2's packages, but I do have automation to rebuild after pushing a patch. So while it takes a little longer, it costs me less of my time on a Friday night... 😄

@landstander668
Copy link
Contributor Author

@dscho So assuming it's OK to pursue switching to the upstream package, I'm having a hard time following how updates to the SDK packages are performed... the structure has changed quite a bit since I last touched it. Can you point me to some information on that front?

I'll try to figure out as much as I can, but I'm a bit lost at the moment.

@dscho
Copy link
Member

dscho commented Mar 1, 2019

I'm having a hard time following how updates to the SDK packages are performed...

The default method is to use pacman -Syu. However, there is some configuration in the SDK (e.g. in /etc/pacman.config's IgnorePkgs line) that you won't get that way, but only by cloning and then pulling. The git pull will obviously only work once the automated job synchronizes the updates to the Git repository (which is done once per night).

In your case, once the package is available, the safest way would be pacman -Sy gawk: update only the gawk package.

@dscho
Copy link
Member

dscho commented Mar 1, 2019

Oh, and gawk v4.2.1-2 is now available. The automation finished.

@landstander668
Copy link
Contributor Author

FYI I decided to simply re-build gawk v4.2.1-2, as I do not have any automation for "unshadowing" MSYS2's packages, but I do have automation to rebuild after pushing a patch.

Sounds good, and has the benefit (to me 😄) of keeping the PR small and simple. I'll get the changes you requested implemented and pushed ASAP.

Update the excluded file patterns, so the contents of "/usr/share/awk"
will be present in the installer and portable builds. This does not
significantly impact the size of the installer, with the full contents
of the directory being approximately 50 KB in size.

This fixes git-for-windows/git#1972.

Signed-off-by: Adric Norris <[email protected]>
@landstander668 landstander668 requested a review from dscho March 2, 2019 04:53
@landstander668 landstander668 changed the title WIP: Enable support for gawk in-place editing Enable support for gawk in-place editing Mar 2, 2019
@landstander668
Copy link
Contributor Author

@dscho I've made the requested changes to the commit and message, and confirmed that the requested feature works after building a new installer and portable release with your SDK update pulled. Since all appears to be well, I've also removed the WIP prefix from the PR, as I believe it to now be in a mergeable state.

The 64-bit installer grew by 18 KB in my testing, which includes approximately 230 KB of shared libraries in /usr/lib/gawk along with the /usr/share/awk contents. So the hit is pretty minimal. 😄

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@dscho dscho merged commit bbe200d into git-for-windows:master Mar 2, 2019
dscho added a commit that referenced this pull request Mar 2, 2019
The `awk` included in Git for Windows [now includes
extensions](#232)
such as `inplace`.

Signed-off-by: Johannes Schindelin <[email protected]>
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.

AWK inplace
2 participants