Concept ACK.

In my take in #23908 I removed the "PACKAGE_NAME" from the SVG completely, since that was the only reason for the whole SVG dance.

Let's do the same thing in this PR since the package name is already shown in the window title and under the icon (see examples below).

See https://github.com/bitcoin/bitcoin/pull/23908/files#diff-ed877ecc59088c3387feeabf1b8bb37da20b3101ea17c8e5d794a7b84f6ebba7 + we could also remove the <style>...</style> at the beginning which only defines the used font.

It is also worth keeping the background.svg in the Git even if it is not used directly.

Before:

After:

It is still quite obvious that Bitcoin Core is about to be installed even without the branding embedded in the background.

Feel free to take the resulting TIFF from https://github.com/prusnak/bitcoin/blob/827a590896acbfd5ef630aaa913e7ca37d1d5f5c/contrib/macdeploy/background.tiff

It is also worth keeping the background.svg in the Git even if it is not used directly.

Agree. And document steps required to convert svg to tiff, that anyone could prove the correctness of the latter.

that anyone could prove the correctness of the latter.

Here is the script I used to generate the TIFF from above:

<background.svg rsvg-convert -f png -d 36 -p 36 -o background.png
<background.svg rsvg-convert -f png -d 72 -p 72 -o [email protected]
optipng -o 7 background*.png  # optional
tiffutil -cathidpicheck background.png [email protected] -out background.tiff

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23778 (release: Guix 1.4.0 & GCC 10.3 by fanquake)
• #21851 (build: support cross-compiling for arm64-apple-darwin by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Is there a reason to add a 30KB file to the repo? If yes, it would be good to explain why and how you generated it.

Locally I am getting 9KB (zip) or 22KB (lzw)

Commands:

tiffcp -c lzw:2 /tmp/pngs/background{,2x}.tiff /tmp/pngs/background.lzw2.tiff
tiffcp -c zip:p9 /tmp/pngs/background{,2x}.tiff /tmp/pngs/background.zip3p9.tiff

Concept ACK 835c308 🦌

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Concept ACK 835c308c6a5b76f07d4e197ef01a4a16edc5e723  🦌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhnbgv/Zbdg7Xzc8xVd7rZlgYaNx6YfMw1OVAhOr8ybZ1WSrN8vuXmgf+IhtI28
x/LosKocFciBnpfz5M8U94FMSL0ck2fOSUDdOMEctop5qmSkc2GVFzkvRwkKhMZq
d7fBFmYBcBL5G42liFxZBjIM1N/0ywZzEA5fHpvelCAMCm2uGAxigpy9zXlIaeQi
DnHth+dfM7lsqRIQIcQGFVIbkzocVU6bpobgHKI7EHjtM9c4qY+QRdrtzNNxABwd
ROyBFWTQInSTLRGWRYXxiXfbDL4tRP49ciu1VK8CzyWNKWvhVF6fa8J72ZDRT7pM
XKRq+o27HGAmrxM1NKW6d2JQDWj6KClVdV8gJJG50fr/5P+xTtqFgCTOap2GHMi/
Hlh9em0MAs6M5/X/MHLotny9lB9Savz40KuZZC7G2G7Isq+mVjDeg2yYAC4TFGdi
H0CNkpUdXyPacIFbMsq6d2OaWDv0ZvU/GiVsroXJEQEP2p0WQlKJYszgaAheSvhF
rQFMLbix
=vah4
-----END PGP SIGNATURE-----

it would be good to explain why and how you generated it.

It came from this branch: https://github.com/prusnak/bitcoin/blob/827a590896acbfd5ef630aaa913e7ca37d1d5f5c/contrib/macdeploy/background.tiff, so I'd assume these were the instructions. I've regenerated it now, and it's about half the size (18KB).

I've also removed the .svg. It doesn't need to remain in the tree, and if anyone wants it they can retrieve it from git (along with the conversion instructions in the makefile).

Added screenshot to PR description, and added @prusnak as co-author given he also opened a PR.

It seems no background in Guix's bitcoin-*-osx-unsigned.dmg.

It seems no background in Guix's bitcoin-*-osx-unsigned.dmg.

Should be fixed now.

CI failure seems related to building out of source tree.

Maybe

--- a/configure.ac
+++ b/configure.ac
@@ -1861,6 +1861,7 @@ AC_CONFIG_LINKS([contrib/devtools/symbol-check.py:contrib/devtools/symbol-check.
 AC_CONFIG_LINKS([contrib/devtools/test-security-check.py:contrib/devtools/test-security-check.py])
 AC_CONFIG_LINKS([contrib/devtools/test-symbol-check.py:contrib/devtools/test-symbol-check.py])
 AC_CONFIG_LINKS([contrib/filter-lcov.py:contrib/filter-lcov.py])
+AC_CONFIG_LINKS([contrib/macdeploy/background.tiff:contrib/macdeploy/background.tiff])
 AC_CONFIG_LINKS([test/functional/test_runner.py:test/functional/test_runner.py])
 AC_CONFIG_LINKS([test/fuzz/test_runner.py:test/fuzz/test_runner.py])
 AC_CONFIG_LINKS([test/util/test_runner.py:test/util/test_runner.py])

?

(did not test though)

Concept NACK. There's no benefit to this; it only serves to sabotage derivative projects.

As mentioned in #23778, there's a straightforward solution to avoid the Rust dependency by simply not using the Rust-based librsvg...

@luke-jr

it only serves to sabotage derivative projects.

This statement is not true. See OP:

Doing this would eliminate the following build dependencies...

Not really. You still need those to actually build it. Now you're just injecting a pre-compiled TIFF into Guix, essentially reverting it back to where things were with gitian (except now the trusted "compiler" isn't necessarily even reproducible).

And these aren't even deps for an actual end-user build, only for producing the release artefacts.

Concept ACK

Reviewers could be interested in historical context: #7192.

There's no benefit to this; it only serves to sabotage derivative projects.

This PR makes it actually easier for derivative projects because they do not need to fiddle with the installer background anymore.

This PR makes it actually easier for derivative projects because they do not need to fiddle with the installer background anymore.

They didn't have to before...

Concept NACK. There's no benefit to this;

I wouldn't have opened a PR if there was no benefit to doing this. As others have mentioned, and as I laid out in the PR description, the benefit is a reduction in the number of dependencies you need to build Bitcoin Core for macOS. In general, if it's possible to simplify a build process with no, or near zero downside (which I certainly think is the case here), then I think it's something we should be doing.

it only serves to sabotage derivative projects.

Not even sure how to respond to this, but I really think you need to get some perspective. What's changing here is outlined in the image below. Remembering that this is a view that's only shown during the install procedure, for a single host, and likely, at most, for the few seconds it takes for a user to drag the app icon over to the folder. Note that the project name is still shown in two other places, so it's not like this is removing the only branding / naming that was present in the DMG. How exactly is anything being sabotaged by this few second view only containing 2 instances of the project name, rather than 3?

Not really. You still need those to actually build it.

No, you don't. Once the .tiff exists in .git, no one has to build it ever again, and thus doesn't need to have those tools present in their build envinronment. That's basically the point of this PR.

Now you're just injecting a pre-compiled TIFF into Guix, essentially reverting it back to where things were with gitian

Comparing the copying of a .tiff into our Guix environment, as the equivalent of reverting back to gitian is absurd.

the benefit is a reduction in the number of dependencies you need to build Bitcoin Core for macOS

You don't need the tiff to build Core (only to package it, which is a special case where deps matter quite a bit less).

How exactly is anything being sabotaged by this few second view only containing 2 instances of the project name, rather than 3?

The description in the PR said the image was added unmodified. Having seen the actual tiff, I see the change, and agree it is less problematic than I originally understood it to be. (However, I still think this isn't a good approach)

Comparing the copying of a .tiff into our Guix environment, as the equivalent of reverting back to gitian is absurd.

It's like adding a compiled bitcoind binary to git and saying that means we don't need GCC/LLVM anymore.

(only to package it, which is a special case where deps matter quite a bit less).

I agree it's a special case, but not in the way you're seemingly dismissing it. The makeup of our Guix build (release packaging) environment is our most important build environment, thus a very special case. I completely disagree with the notion that "deps matter quite a bit less" there, in fact, I think the opposite is true. In that environment, every additional dependency (and the dependencies of those dependencies) increases the scope for potential compromise, and/or tool/supply chain related attacks and are just more things we have to potentially maintain / vet / install. So I think any change that reduces the build requirements for that environment, is a good one. In this case, we can drop imagemagick, libtiff, librsvg and font-tuffy when performing the macOS cross-compile.

The description in the PR said the image was added unmodified.

Apologies, I'll update the PR description to reflect the current change. However, there were at least 4 screenshots, showing the new DMG view, posted in this thread before you made your comments.

Post-merge ACK e09773d. Much better than requiring rust, and a nice simplification.

Post-merge ACK e09773d

I concur