Page MenuHomeFreeBSD

Tracker for mono-related reviews
AbandonedPublic

Authored by dbn on Sep 21 2017, 8:18 PM.

Details

Reviewers
russ.haley_gmail.com
Group Reviewers
mono
Summary

This review has been split into multiple parts
This review remains as a meta review to track the multiple parts

Individual reviews:

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Could you write a summary of what you are doing ? (one that is human readable, that is)

Also, could you use devel/arcanist so that we get the actual commits you are talking about in here, and the context, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.

romain edited the summary of this revision. (Show Details)
romain edited the summary of this revision. (Show Details)
romain retitled this revision from Update mono to 5.2.0.215 to Update lang/mono to 5.2.0.215 and update related ports.Sep 22 2017, 9:42 AM
romain edited the summary of this revision. (Show Details)
dbn edited the summary of this revision. (Show Details)

Changes:

  • rebase patch to r450479
  • lang/mono: fix distinfo with ACCEPTANCE_TESTS
  • spelling fix in summary
  • update log in summary
dbn edited the summary of this revision. (Show Details)
dbn edited the test plan for this revision. (Show Details)

Rebase to r453757

Nice job adding msbuild. That's going to come in handy for dotnet core 2. I'll try building this again asap.

Mk/Uses/mono.mk
57 ↗(On Diff #34954)

Just a sanity check that mono_ARGS:Nnuget and mono_ARGS:Mnuget on line 77 are actually two different things and not a typo?

132 ↗(On Diff #34954)

Can you please give me a brief on what this section is doing?

devel/monodevelop/Makefile
30 ↗(On Diff #34954)

Are we pulling a nuget package called appveyor or is that the name of the site we pull nugetizer3000 from? If the later, it doesn't follow your "naming convention". Also, it seems the NUGET_URL is defined somewhere else. Is that on purpose or would it be better to put it here too?

devel/monodevelop/nupkg-nuget
124 ↗(On Diff #34954)

What are these Linux packages for?

dbn marked 3 inline comments as done.Nov 25 2017, 2:35 PM
dbn added inline comments.
Mk/Uses/mono.mk
57 ↗(On Diff #34954)

Yes, this line should use :N and line 77 should use :M. The former means "all that don't match" whereas the latter means "all that match". Sanity checked ;-)

132 ↗(On Diff #34954)

Nuget "caches" (I think nuget uses a different term) the packages in an extracted format, plus a few extra files:

  • the package stored at ${PACKAGE}.nupkg; and
  • the package checksum stored at ${PACKAGE}.nupkg.sha512

The above line calculates the cache in base64 format, without trailing space. I would have used sha512 however our implemntationd doesn't support base64 output.

devel/monodevelop/Makefile
30 ↗(On Diff #34954)

Neither :-/. We are pulling a package called NuGet.Build.Packaging. It is being pulled from a site called nugetizer3000? hosted at appveyor. What name do you think is intuitive?

devel/monodevelop/nupkg-nuget
124 ↗(On Diff #34954)

I'll explain the full context of how this list is generated, but in brief: because that is what the port's build system expects. These packages are not used.

devel/monodevelop/Makefile
30 ↗(On Diff #34954)

https://github.com/NuGet/NuGet.Build.Packaging
Nuget.Build.Packaging is the namespace for the Nugetizer3000 software.

If the AZURE_APPSERVICE_URL pulls the azure app service, the TEMPLATING_URL is for the templating engine, ROSLYN_URL pulls the compiler, then naming the url for Build Packaging for by the hosting site is incongruous with the other names. NUGETIZER3000_URL or NUGET_BUILD_PACK_URL or some variant there of would be more appropriate in my opinion. :)

devel/msbuild/files/csc.exe
1 ↗(On Diff #34954)

csc.exe is the C# compiler. Can I ask why there is a python file named csc.exe in the files/ dir? Not that it's definitive, but I don't see anything like this in Microsoft.msbuild gh repo.

devel/monodevelop/nupkg-nuget
124 ↗(On Diff #34954)

So the upstream package requires them but doesn't use them? That's fine I guess, I'm just being curious.

Hi David,

In the end, this review is just too massive for me to work with. I had some issues with the patch for mono but was able to get it to build. I'd like to help and submit that patch, but I can't work with all the other changes too. Can I suggest breaking this review up along the groupings in your comment and submitting each part separately (starting with the mono update)? I just don't think this update will ever get finished due to it's breadth.

dbn edited the test plan for this revision. (Show Details)
dbn edited the test plan for this revision. (Show Details)
dbn marked 9 inline comments as done.EditedJan 3 2018, 7:33 PM

Hi David,

In the end, this review is just too massive for me to work with. I had some issues with the patch for mono but was able to get it to build. I'd like to help and submit that patch, but I can't work with all the other changes too. Can I suggest breaking this review up along the groupings in your comment and submitting each part separately (starting with the mono update)? I just don't think this update will ever get finished due to it's breadth.

TL;DR: I completely agree.

  • I suggest you review mono (https://reviews.freebsd.org/D13752)
  • I'll check/commit the ancillary updates
  • Please subscribe yourselves to the relevant dependent reviews (more to come - see summary)

I tried splitting up this patch, and yes, it is very big and complex :-(. What I've done is turn this review into a meta review and created two reviews for the pieces of work that can be committed (independently). I've kept the content of this review so we don't lose anything raised.

The other three items depend directly and indirectly on mono so I suggest we delay reviewing them until we get mono reviewed. I'm happy to manage this process as I somewhat understand.

dbn removed a reviewer: portmgr.

Yay, ancillary ports have been committed. I'm slowly flushing out the backlog of changes that I've accumulated.

dbn retitled this revision from Update lang/mono to 5.2.0.215 and update related ports to Tracker for mono-related reviews.Jan 11 2018, 6:37 PM
dbn edited the summary of this revision. (Show Details)
dbn removed a reviewer: portmgr.
dbn added a subscriber: portmgr.
dbn edited reviewers, added: mono; removed: portmgr.

Trim patch
Add mono group

I plan to commit this tomorrow (in time for the quarterly branch)

All dependent reviews have been committed. We are now up-to-date with VS for Mac (give or take msbuild).