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

Add the mingw-w64-7zip package #13894

Merged
merged 2 commits into from
Nov 4, 2022
Merged

Add the mingw-w64-7zip package #13894

merged 2 commits into from
Nov 4, 2022

Conversation

dscho
Copy link
Contributor

@dscho dscho commented Nov 3, 2022

MSYS2 does offer the p7zip package. But it is an MSYS package (and therefore the executable is slower than it needs to be), and besides, the p7zip project seems to be abandoned since 2016.

In the meantime, the 7-Zip project itself added support for building the command-line tool on Linux, and we can use that very same setup to build a MINGW version, too. So let's do that.

Note that there is only a 7z.exe, no 7za.exe. That's because the 7-Zip project never bothered with a shared library ;-)

@Biswa96
Copy link
Member

Biswa96 commented Nov 3, 2022

clang builds fail with -Werror. It is allowed to remove the -Werror flag with sed or a patch file.

MSYS2 does offer the `p7zip` package. But it is an MSYS package (and
therefore the executable is slower than it needs to be), and besides,
the p7zip project seems to be abandoned since 2016.

In the meantime, the 7-Zip project itself added support for building the
command-line tool on Linux, and we can use that very same setup to build
a MINGW version, too. So let's do that.

Signed-off-by: Johannes Schindelin <[email protected]>
@jeremyd2019
Copy link
Member

awesome. msys2-installer uses a 7z sfx, is it possible to build that here rather than downloading a binary from elsewhere?

Suggested-by: Jeremy Drake <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Contributor Author

dscho commented Nov 3, 2022

awesome. msys2-installer uses a 7z sfx, is it possible to build that here rather than downloading a binary from elsewhere?

I guess it is possible to build that, too ;-)

@dscho
Copy link
Contributor Author

dscho commented Nov 3, 2022

awesome. msys2-installer uses a 7z sfx, is it possible to build that here rather than downloading a binary from elsewhere?

I guess it is possible to build that, too ;-)

@jeremyd2019 please do note, though, that this seems not to include zstd support (but given its weaker compression compared to LZMA, that might not be a concern anyway).

@dscho dscho marked this pull request as ready for review November 3, 2022 22:42
@jeremyd2019
Copy link
Member

might be a worthwhile tradeoff to be able to use an sfx built here.

@lazka lazka merged commit 51ba848 into msys2:master Nov 4, 2022
@lazka
Copy link
Member

lazka commented Nov 4, 2022

I switched it to zstd for faster decompression/installation: msys2/msys2-installer#37

But if the external download is a concern, I'm fine with reverting that.

@lazka
Copy link
Member

lazka commented Nov 4, 2022

For future reference, arm64 failed with:

../../../../C/7zCrc.c:158:1: error: unknown architecture 'armv8-a+crc' in the 'target' attribute string; 'target' attribute ignored [-Werror,-Wignored-attributes]
ATTRIB_CRC
^
../../../../C/7zCrc.c:89:58: note: expanded from macro 'ATTRIB_CRC'
            #define ATTRIB_CRC __attribute__((__target__("arch=armv8-a+crc")))
                                                         ^
../../../../C/7zCrc.c:160:1: error: unknown architecture 'armv8-a+crc' in the 'target' attribute string; 'target' attribute ignored [-Werror,-Wignored-attributes]
ATTRIB_CRC
^
../../../../C/7zCrc.c:89:58: note: expanded from macro 'ATTRIB_CRC'
            #define ATTRIB_CRC __attribute__((__target__("arch=armv8-a+crc")))
                                                         ^
../../../../C/7zCrc.c:190:1: error: unknown architecture 'armv8-a+crc' in the 'target' attribute string; 'target' attribute ignored [-Werror,-Wignored-attributes]
ATTRIB_CRC
^
../../../../C/7zCrc.c:89:58: note: expanded from macro 'ATTRIB_CRC'
            #define ATTRIB_CRC __attribute__((__target__("arch=armv8-a+crc")))
                                                         ^
../../../../C/7zCrc.c:192:1: error: unknown architecture 'armv8-a+crc' in the 'target' attribute string; 'target' attribute ignored [-Werror,-Wignored-attributes]
ATTRIB_CRC
^
../../../../C/7zCrc.c:89:58: note: expanded from macro 'ATTRIB_CRC'
            #define ATTRIB_CRC __attribute__((__target__("arch=armv8-a+crc")))
                                                         ^
4 errors generated.
make: *** [../../7zip_gcc.mak:1064: _o/7zCrc.o] Error 1
make: *** Waiting for unfinished jobs....

I'm going to disable it for now.

lazka added a commit that referenced this pull request Nov 4, 2022
@dscho
Copy link
Contributor Author

dscho commented Nov 4, 2022

Strange. I specifically fixed the ARM64 build yesterday, by disabling some overzealous errors...

@dscho dscho deleted the 7zip branch November 4, 2022 10:58
@dscho
Copy link
Contributor Author

dscho commented Nov 4, 2022

I switched it to zstd for faster decompression/installation: msys2/msys2-installer#37

But if the external download is a concern, I'm fine with reverting that.

Now that we have a working definition, we could add the patch to add zstd support...

@lazka
Copy link
Member

lazka commented Nov 4, 2022

Now that we have a working definition, we could add the patch to add zstd support...

From a quick look the fork only added it to the MSVC build scripts, so that would need some extra work.

Upstream seems to be working on zstd support though, so we could also just wait for that: mcmilk/7-Zip-zstd#271

@dscho
Copy link
Contributor Author

dscho commented Nov 4, 2022

Upstream seems to be working on zstd support though, so we could also just wait for that: mcmilk/7-Zip-zstd#271

@lazka that's probably the safest.

I did have a quick look at the diff between 7-Zip and 7-Zip-zstd and it looks quite large, in particular because it also brings brotli, fast-lzma2, lizard and updated icons (see for yourself: git-for-windows/7-Zip@f8a8f0d). Also, I am not quite sure about licensing, it looks as if zstd is under the GPL but 7-Zip-zstd seems to ignore that and treat it as LGPL. But IANAL nor I do I want to be.

So the best would most likely be to keep using the current binary download of 7zCon.sfx up until the time when 7-Zip adds Zstandard support.

@jeremyd2019
Copy link
Member

jeremyd2019 commented Nov 4, 2022

For future reference, arm64 failed with:

../../../../C/7zCrc.c:158:1: error: unknown architecture 'armv8-a+crc' in the 'target' attribute string; 'target' attribute ignored [-Werror,-Wignored-attributes]

I'm going to disable it for now.

Hmm, I just googled it and it looked like clang was supposed to support that construct. @mstorsjo do you know a reason it wouldn't on windows/mingw? Or do we need to patch it to use a different way to enable crc instructions for clang there?

EDIT: clang was supposed to support that construct in -march= maybe it doesn't also support it in the attribute?

@mstorsjo
Copy link
Contributor

mstorsjo commented Nov 4, 2022

For future reference, arm64 failed with:

../../../../C/7zCrc.c:158:1: error: unknown architecture 'armv8-a+crc' in the 'target' attribute string; 'target' attribute ignored [-Werror,-Wignored-attributes]

I'm going to disable it for now.

Hmm, I just googled it and it looked like clang was supposed to support that construct. @mstorsjo do you know a reason it wouldn't on windows/mingw? Or do we need to patch it to use a different way to enable crc instructions for clang there?

EDIT: clang was supposed to support that construct in -march= maybe it doesn't also support it in the attribute?

It looks like this exact format of the attribute does work in latest git of clang, but not in the current 15.0 release.

Using __attribute__((__target__("crc"))) does seem to work with existing Clang releases, but GCC doesn't accept that form on the other hand.

I've bisected down the interesting changes between 15.0 and the current git main branch; it's primarily llvm/llvm-project@781b491 which affects this, while llvm/llvm-project@30b67c6 also has a bit of an effect here:

commit 781b491bba9d798e53f7784dced3c2be77c81dd4
Author: David Green <[email protected]>
Date:   Sat Oct 1 15:40:59 2022 +0100

    [Clang][AArch64] Support AArch64 target(..) attribute formats.
    
    This adds support under AArch64 for the target("..") attributes. The
    current parsing is very X86-shaped, this patch attempts to bring it line
    with the GCC implementation from
    https://gcc.gnu.org/onlinedocs/gcc/AArch64-Function-Attributes.html#AArch64-Function-Attributes.
    
    The supported formats are:
    - "arch=<arch>" strings, that specify the architecture features for a
      function as per the -march=arch+feature option.
    - "cpu=<cpu>" strings, that specify the target-cpu and any implied
      atributes as per the -mcpu=cpu+feature option.
    - "tune=<cpu>" strings, that specify the tune-cpu cpu for a function as
      per -mtune.
    - "+<feature>", "+no<feature>" enables/disables the specific feature, for
      compatibility with GCC target attributes.
    - "<feature>", "no-<feature>" enabled/disables the specific feature, for
      backward compatibility with previous releases.
    
    To do this, the parsing of target attributes has been moved into
    TargetInfo to give the target the opportunity to override the existing
    parsing. The only non-aarch64 change should be a minor alteration to the
    error message, specifying using "CPU" to describe the cpu, not
    "architecture", and the DuplicateArch/Tune from ParsedTargetAttr have
    been combined into a single option.
    
    Differential Revision: https://reviews.llvm.org/D133848

and

commit 30b67c677c6baf0d6ef6c3051cf270133c43e4d2
Author: Daniel Kiss <[email protected]>
Date:   Fri Oct 14 17:21:17 2022 +0200

    [AArch64] Make ACLE intrinsics always available part1

    A given arch feature might enabled by a pragma or a function attribute so in this cases would be nice to use intrinsics.
    Today GCC offers the intrinsics without the march flag[1].
    PR[2] for ACLE to clarify the intention and remove the need for -march flag for a given intrinsics.

    This is going to be more useful when D127812 lands.

    [1] https://godbolt.org/z/bxcMhav3z
    [2] https://github.com/ARM-software/acle/pull/214

    Reviewed By: dmgreen

    Differential Revision: https://reviews.llvm.org/D133359

@jeremyd2019
Copy link
Member

So maybe patching it to use __attribute__((__target__("crc"))) with a comment that the patch should be dropped once clang is updated to 16 would be the thing to do.

dscho added a commit to dscho/build-extra that referenced this pull request Aug 25, 2023
Back in the days, 7-Zip was only a Windows GUI program, and p7zip was
started as a fork to provide a CLI program that could also be compiled
and used on Linux. Since it was easier to port p7zip than 7-Zip to
MSYS2, we used the former to build the PortableGit, and even to squeeze
out more compression for the MinGit `.zip`.

Sadly, p7zip is languishing. The latest version of that project is from
2016: https://sourceforge.net/projects/p7zip/files/.

Happily, 7-Zip was modified in the meantime to allow building CLI
programs on Linux, and based on that support, I was able to port it to
MSYS2: msys2/MINGW-packages#13894.

Yesterday, I updated it to match 7-Zip v23.01:
msys2/MINGW-packages#18254, and this is as fine
an opportunity as any to start moving away from a stale p7zip.

This should also be a bit quicker, as we're moving from an MSYS2
`7za.exe` to a MINGW `7z.exe`.

Also, 7-Zip v23.01 improved the compression. Not by a whole lot
(PortableGit shrank by about 200kB, which is not much given that the
entire file is around 57MB to begin with), but hey, it's something.

Note: The change to `portable/release.sh` is a bit more complicated
because 1) the command-line got too long (MSYS2 side-steps the 32-kB
length limit by playing games with shared memory), and 2) the absolute
path needs to be provided as a _Windows_ path now, not as a pseudo-Unix
path.

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.

5 participants