Improve D23300

[mikael]

Summary

After five months of waiting, I'm desperately trying make this patch happen.

All credit for the original work goes to @prj_rootwyrm.com of course.

My changes:

• not changing the ownership because that would be weird if I'm submitting the patch
• add a patch to replace /usr/share with /usr/local/share
• remove the --quiet flag from cert-sync as this also suppresses error reporting which can lead to mysterious TLS errors later on in case it failed

And as requested in the most recent comment:

• update the aarch64-btls patch
• add pkg-plist.powerpc
• move options
• remove the iconv dependency

Tagged reviewers and some subscribers from the D23300. Apologies if I messed up, I'm not very familiar with Phabricator.

[val_packett.cool]

Nice.

btls patch doesn't look updated, where's the ifndef ID_AA64ISAR0_AES_VAL stuff?

https://github.com/mono/boringssl/pull/23/files is the source for the final patch

I'm not very familiar with Phabricator

If you were, you'd know you could steal D23300 by clicking "Add action.." -> "Commandeer Revision" above the comment form :) and then updating its diff

[noah.bergbauer_tum.de]

Actually update to the corret btls patch

[noah.bergbauer_tum.de]

By the way, this breaks the devel/msbuild port, because the old msbuild version is not compatible with the newer compiler version. Given that it relies on pulling a plethora of binary dependencies from nuget anyways, there is arguably little point in having it actually compile msbuild. On Arch Linux there is a package that simply pulls in http://download.mono-project.com/repo/ubuntu/pool/main/m/msbuild/msbuild_16.5+xamarinxplat.2020.02.20.11.54-0xamarin2+ubuntu2004b1_all.deb (ubuntu package provided by the mono project). This runs just fine on FreeBSD with only two changes:

1. Rename all files from /usr/ to /usr/local/
2. Patch the two paths in the /usr/local/bin/msbuild script to also be prefixed with /usr/local

I think the msbuild port should simply take this approach. I just don't know how to make it do this, so I would greatly appreciate some advice.

[pkubaj]

Looking at the diff between pkg-plist and pkg-plist.powerpc, I noticed I missed this at the end of pkg-plist.powerpc:

@postexec %%PREFIX%%/bin/cert-sync %%LOCALBASE%%/share/certs/ca-root-nss.crt
@postunexec rm -rf %%LOCALBASE%%/share/.mono

Please add this.

[noah.bergbauer_tum.de]

Update pkg-plist.powerpc as requested

[noah.bergbauer_tum.de]

Add msbuild patch

[mikael]

devel/msbuild/Makefile

15

Why is it commented?

24

@${DO_NADA}

25–28

don't hardcode /usr/local, use ${LOCALBASE} instead (or it has to live in /usr/local?)

Is /usr/local/bin/msbuild a binary or a shell script (I haven't looked, it seems weird to SED a string in a binary). Also why not REINPLACE_CMD?

${TAR} -xOf ${_DISTDIR}/${DISTFILES} data.tar.xz | ...

30

Why not a pkg-plist?

lang/mono/Makefile

28–31

extra space at EOL?

[noah.bergbauer_tum.de]

devel/msbuild/Makefile

25–28

It's a shell script that invokes mono. I didn't know about REINPLACE_CMD.

${TAR} -xOf ${_DISTDIR}/${DISTFILES} data.tar.xz | ...

I'm not sure what you mean. Are you telling me to extract the deb file with tar instead of ar? Because ar is also what net-mgmt/unifi-lts uses (I used it as a template).

30

I'm under the impression that the point of a pkg-plist file is to select what files you actually want to add. In this case the selection was already made when upstream built their debian package.

AFAICS repeating the exact list of files from inside the tar archive in a plist file over here is essentially just creating busywork.

I copied this find-tmpplist construction from net-mgmt/unifi-lts which I used as a template.

[mikael]

devel/msbuild/Makefile

25–28

yes, it seems weird to use 'ar' and then 'tar'

30

the pkg-plist is explained in porters handbook section 3.2.2. pkg-plist
The static pkg-plist is preferred:
plist-dynamic section 8.4. Dynamic Versus Static Package List

[noah.bergbauer_tum.de]

Implement changes requested in review

[mikael]

There are a bunch of portlint issues that can be fixed:

portlint -AC

[noah.bergbauer_tum.de]

Fix portlint issues

[mikael]

Do the same for lang/mono please.

[noah.bergbauer_tum.de]

In D24988#551785, @mikael wrote:

Do the same for lang/mono please.

Would you mind elaborating? I'm not familiar with any of this.

installing gettext translation files, please define USES[+]=gettext as appropriate

Do I just blindly add USES=gettext?

use of DISTFILES with single file discouraged. distribution filename should be set by DISTNAME and EXTRACT_SUFX.
WARN: Makefile: DISTFILES/DISTNAME affects WRKSRC. take caution when changing them.

I guess I just ignore this?

WARN: Makefile: "BROKEN" has to appear earlier.
WARN: Makefile: "NOT_FOR_ARCHS" has to appear earlier.
WARN: Makefile: "BROKEN" has to appear earlier.
WARN: Makefile: "NOT_FOR_ARCHS" has to appear earlier.
WARN: Makefile: "LIB_DEPENDS" has to appear earlier.
WARN: Makefile: "BUILD_DEPENDS" has to appear earlier.
WARN: Makefile: "RUN_DEPENDS" has to appear earlier.
WARN: Makefile: "USES" has to appear earlier.

No idea what this means, just tell me where to move things.

WARN: /usr/ports/lang/mono/files/patch-btls-aarch64: patch was not generated using `make makepatch''. It is recommended to use `make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.
WARN: /usr/ports/lang/mono/files/patch-inotify: patch was not generated using `make makepatch''. It is recommended to use `make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.
WARN: /usr/ports/lang/mono/files/patch-mcs_class_corlib_System_Environment.cs: patch was not generated using `make makepatch''. It is recommended to use `make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.

No idea what to do with these.

[mikael]

In D24988#551787, @noah.bergbauer_tum.de wrote:

installing gettext translation files, please define USES[+]=gettext as appropriate

Do I just blindly add USES=gettext?

This one can be ignored (it's handled by NLS_USES= gettext) see using gettext

use of DISTFILES with single file discouraged. distribution filename should be set by DISTNAME and EXTRACT_SUFX.
WARN: Makefile: DISTFILES/DISTNAME affects WRKSRC. take caution when changing them.

I guess I just ignore this?

Yes

WARN: Makefile: "BROKEN" has to appear earlier.
WARN: Makefile: "NOT_FOR_ARCHS" has to appear earlier.
WARN: Makefile: "BROKEN" has to appear earlier.
WARN: Makefile: "NOT_FOR_ARCHS" has to appear earlier.
WARN: Makefile: "LIB_DEPENDS" has to appear earlier.
WARN: Makefile: "BUILD_DEPENDS" has to appear earlier.
WARN: Makefile: "RUN_DEPENDS" has to appear earlier.
WARN: Makefile: "USES" has to appear earlier.

No idea what this means, just tell me where to move things.

Good question, it seems to be in the correct order porting-order. Just ignore it.

WARN: /usr/ports/lang/mono/files/patch-btls-aarch64: patch was not generated using `make makepatch''. It is recommended to use `make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.
WARN: /usr/ports/lang/mono/files/patch-inotify: patch was not generated using `make makepatch''. It is recommended to use `make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.
WARN: /usr/ports/lang/mono/files/patch-mcs_class_corlib_System_Environment.cs: patch was not generated using `make makepatch''. It is recommended to use `make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.

No idea what to do with these.

I never understood why the patch header has to be in a certain form, either regenerate them with make makepatch or fix it manually, the header should look like this

--- configure.ac.orig   2020-03-15 16:11:23 UTC
+++ configure.ac

[noah.bergbauer_tum.de]

Fix portlint issues for lang/mono

[mikael]

lang/mono/Makefile

20

Just list the available arch:
ONLY_FOR_ARCHS= aach64 amd64 powerpc64 (maybe i386)

[noah.bergbauer_tum.de]

Switch to ONLY_FOR_ARCHS as requested

[prj_rootwyrm.com]

In D24988#550221, @pkubaj wrote:

Looking at the diff between pkg-plist and pkg-plist.powerpc, I noticed I missed this at the end of pkg-plist.powerpc:

@postexec %%PREFIX%%/bin/cert-sync %%LOCALBASE%%/share/certs/ca-root-nss.crt
@postunexec rm -rf %%LOCALBASE%%/share/.mono

Please add this.

This is incorrect and it must not be in pkg-plist.powerpc, because pkg-plist.powerpc is strictly an append. Everything in pkg-plist is for 100% of platforms; ppc64 generates additional binaries, not replacement binaries.
Do NOT duplicate the @postunexec under ANY circumstances no matter what. Because this breaks both upgrading the package and NuGet consumers. I am still working with a solution that will address NuGet consumers, because this is getting into host-dynamic plists and no you cannot wildcard.

Also, there is a reason msbuild is to not be used for any reason: it not only does but will break. msbuild has never been tested beyond "yes, everything went terribly." It has never been in CI and will never be in CI, either. All CI and unit testing is done with monolite. msbuild is just a thing that exists.

All that said, I hate to be the bearer of bad news but 1) 6.8.0.105 is (likely) out of date at this point 2) 6.8.0.105 with the BTLS patches is showing a severe regression in BTLS itself now, which is not proving fun to track down 3) 6.8.0.123 has other severe issues I'm working through around solving the sgen problems. I asked the VMM people for guidance and they didn't care to come down from their ivory towers, so now it's gonna be whatever I throw together and that's it. And this is a MUST FIX due to catastrophic sgen performance regression without it. (Oh, and blowing up the VMM system. No big, right? )

System.Net.WebException: Error: TrustFailure (Authentication failed, see inner exception.):  [redacted] System.Net.WebException: Error: TrustFailure (Authentication failed, see inner exception.) ---> System.Security.Authentication.AuthenticationException: Authentication failed, see inner exception. ---> Mono.Btls.MonoBtlsException: Ssl error:1000007d:SSL routines:OPENSSL_internal:CERTIFICATE_VERIFY_FAILED
at /wrkdirs/usr/ports/lang/mono/work/mono-6.8.0.105/external/boringssl/ssl/handshake_client.c:1132
  at Mono.Btls.MonoBtlsContext.ProcessHandshake () [0x00048] in <88c7d2e4b4df4e27baa411ae9198218c>:0 
  at Mono.Net.Security.MobileAuthenticatedStream.ProcessHandshake (Mono.Net.Security.AsyncOperationStatus status, System.Boolean renegotiate) [0x000da] in <88c7d2e4b4df4e27baa411ae9198218c>:0 
  at (wrapper remoting-invoke-with-check) Mono.Net.Security.MobileAuthenticatedStream.ProcessHandshake(Mono.Net.Security.AsyncOperationStatus,bool)
  at Mono.Net.Security.AsyncHandshakeRequest.Run (Mono.Net.Security.AsyncOperationStatus status) [0x00006] in <88c7d2e4b4df4e27baa411ae9198218c>:0 
  at Mono.Net.Security.AsyncProtocolRequest.ProcessOperation (System.Threading.CancellationToken cancellationToken) [0x000fc] in <88c7d2e4b4df4e27baa411ae9198218c>:0 
   --- End of inner exception stack trace ---

The failure here occurs on amd64, not a tier 2 architecture, so something has gone very wrong but frankly I don't know anywhere near enough about btls to guess what.

[noah.bergbauer_tum.de]

In D24988#552276, @prj_rootwyrm.com wrote:

Also, there is a reason msbuild is to not be used for any reason: it not only does but will break. msbuild has never been tested beyond "yes, everything went terribly." It has never been in CI and will never be in CI, either. All CI and unit testing is done with monolite. msbuild is just a thing that exists.

Perhaps there is some miscommunication here, but msbuild works just fine for me. If you're talking about building mono with msbuild then please note that I haven't touched your patch at all except for changes I listed above.

All that said, I hate to be the bearer of bad news but 1) 6.8.0.105 is (likely) out of date at this point 2) 6.8.0.105 with the BTLS patches is showing a severe regression in BTLS itself now, which is not proving fun to track down 3) 6.8.0.123 has other severe issues I'm working through around solving the sgen problems. I asked the VMM people for guidance and they didn't care to come down from their ivory towers, so now it's gonna be whatever I throw together and that's it. And this is a MUST FIX due to catastrophic sgen performance regression without it. (Oh, and blowing up the VMM system. No big, right? )

Hm, the diff to 6.8.0.123 seems fairly small. Do you have any details?

The failure here occurs on amd64, not a tier 2 architecture, so something has gone very wrong but frankly I don't know anywhere near enough about btls to guess what.

Is this the .105 BTLS regression you were talking about? Because I used to get this error when the cert-sync postexec task failed (and hence no certificates were installed). This is why I removed the --quiet flag which sadly silences all output, both success and errors (which would be important to know).

The bottom line is that I haven't run into any of the catastrophic problems you're talking about - after a week of using it every day (on amd64). So if you have more information, perhaps I could try to reproduce and debug? And just in case this helps you, here is my mono package and this is msbuild.

[mikael]

committed in https://svnweb.freebsd.org/ports?view=revision&revision=557880