Skip to content

[builds] Make the tools build use mono's packaged logic instead of our own. - #5962

Merged
rolfbjarne merged 4 commits into
dotnet:masterfrom
rolfbjarne:mono-binaries-bcl-packaged-src-build
Apr 30, 2019
Merged

[builds] Make the tools build use mono's packaged logic instead of our own.#5962
rolfbjarne merged 4 commits into
dotnet:masterfrom
rolfbjarne:mono-binaries-bcl-packaged-src-build

Conversation

@rolfbjarne

Copy link
Copy Markdown
Member

Make the 'tools64' build use mono's packaged build logic instead of our own.
This is the first step to consuming the BCL from the mono archive.

Also completely refactor the 'tools64' build by removing everything we don't
need and renaming it to 'bcl' (since that's more representative of what it
does).

This also means we're now consuming the BCL from the mono archive.

…r own.

Make the 'tools64' build use mono's packaged build logic instead of our own.
This is the first step to consuming the BCL from the mono archive.

Also completely refactor the 'tools64' build by removing everything we don't
need and renaming it to 'bcl' (since that's more representative of what it
does).
Move the facade check targets below the declaration of their prerequisite
variables (*_BCL_TARGETS), since otherwise the prerequisite variable will be
empty when the facade check targets are read by make, they end up with no
prerequisites at all, and the targets fail.
@rolfbjarne
rolfbjarne requested a review from spouliot as a code owner April 29, 2019 05:27
@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Build failed or was aborted

Build succeeded
🔥 Failed to create API Diff 🔥

Before we unzip, we remove the target directory. This is a bad idea if the
target directory is also used for other things: in particular it breaks
parallel make if some other target tries to write to the temporary directory.

Instead unzip downloaded files into a subdirectory exclusively used by those
unzipped files, which means we can remove at will before unzipping.

@akoeplinger akoeplinger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very nice!

Comment thread builds/Makefile
@@ -328,8 +326,8 @@ $(MAC_DIRECTORIES) $(BUILD_DESTDIR):
mac-facade-check: $(MAC_BCL_TARGETS)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We'll need to get rid of the FACADE_SUBDIRS_MK which is included directly from mono sources in https://github.com/xamarin/xamarin-macios/blob/03669d4c030b2313d2037493307e3cb2d4f3bd88/builds/Makefile#L188

There's two options: 1) just include all *.dll in the ios/mac-bcl folders and trust mono to not put unwanted files there 2) hardcode list of assemblies/facades here. Any preferences?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think the Xamarin.Linker.SdkTest should cover this scenario, so I think we can just remove the whole facade check from this makefile.

@spouliot what do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually the facade check is just one part of the problem, we'd still need a list of the facade assemblies. Solution 1) won't work when building from source, so I think we need a dual solution: keep the existing code when building from source, and then use solution 1) when we're using the mono archive.

We still have a hard-coded list of facade assemblies elsewhere in our code (in tests), so we'll be notified if new facade assemblies are added to the archive (the corresponding test will start to fail).

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    Failed to build src/
    Search for Comparing API & creating generator diff in the log to view the complete log.
Test run succeeded

@rolfbjarne rolfbjarne added the skip-api-comparison Skips API / generator diffs when testing pull requests label Apr 29, 2019
@rolfbjarne

Copy link
Copy Markdown
Member Author

The api comparison failed because the src build is too different from the previous commit: i.e. the failure is expected/can be ignored.

Comment thread builds/Makefile

$(BUILD_DESTDIR)/ios-bcl/%.dll: $(MONO_IOS_SDK_DESTDIR)/ios-bcl/%.dll | $(TMP_BCL_TARGET_DIRS)
$(Q) $(CP) $< $@
$(Q_SN) MONO_CFG_DIR=$(TOP) $(SYSTEM_SN) -q -R $@ $(PRODUCT_KEY_PATH)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need to re-sign here? the assemblies from the mono archive should already have the correct signature (if they don't I'd consider that a bug).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think we do need to re-sign. I'll have a look and put up another PR if that's the case.

@rolfbjarne
rolfbjarne merged commit b72a0d9 into dotnet:master Apr 30, 2019
rolfbjarne added a commit that referenced this pull request Apr 30, 2019
spouliot pushed a commit that referenced this pull request Apr 30, 2019
akoeplinger added a commit to akoeplinger/macios that referenced this pull request May 12, 2019
akoeplinger added a commit that referenced this pull request May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-api-comparison Skips API / generator diffs when testing pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants