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.

Sign up for GitHub

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

Jump to bottom

Replace uses of CONFIGURATIONS by CONFIGURATION #2325

Merged
merged 2 commits into from
Oct 25, 2018
Merged

Replace uses of CONFIGURATIONS by CONFIGURATION #2325

merged 2 commits into from
Oct 25, 2018

Conversation

luhenry
Copy link
Contributor

@luhenry luhenry commented Oct 19, 2018

No description provided.

@luhenry luhenry added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Oct 19, 2018
@luhenry luhenry requested a review from jonpryor October 19, 2018 20:26
@jonpryor
Copy link
Member

The macOS PR Build failed:

_RemapAssemblies:
  mono "…/xamarin-android/bin/BuildRelease/remap-assembly-ref.exe" "../../../bin/Release/bcl-tests/monodroid_corlib_test.dll" "obj/Release/monodroid_corlib_test.dll" nunitlite "…/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/Xamarin.Android.NUnitLite.dll"
  Cannot open assembly '/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/BuildRelease/remap-assembly-ref.exe': No such file or directory.
…/tests/BCL-Tests/Xamarin.Android.Bcl-Tests/Xamarin.Android.Bcl-Tests.targets(28,5): error MSB3073: The command "mono "/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/BuildRelease/remap-assembly-ref.exe" "../../../bin/Release/bcl-tests/monodroid_corlib_test.dll" "obj/Release/monodroid_corlib_test.dll" nunitlite "…/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/Xamarin.Android.NUnitLite.dll"" exited with code 2.

Looks like we missed a spot, and it's trying to use BuildRelease when building the Debug configuration.

jonpryor
jonpryor reviewed Oct 22, 2018
View reviewed changes
Makefile Outdated
(cd $(call GetPath,JavaInterop) && make prepare CONFIGURATION=$(conf) JI_MAX_JDK=8) && \
(cd $(call GetPath,JavaInterop) && make bin/Build$(conf)/JdkInfo.props CONFIGURATION=$(conf) JI_MAX_JDK=8) && ) \
true
(cd external/xamarin-android-tools && make prepare CONFIGURATION=$(CONFIGURATION)) && \
Copy link
Member

Choose a reason for hiding this comment

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

There's no longer a need for \/line continuation. These can all be separate invocations, and let make bail whenever any command fails.

radekdoulik
radekdoulik reviewed Oct 23, 2018
View reviewed changes
build-tools/scripts/BuildEverything.mk
@@ -80,8 +80,6 @@ _MSBUILD_ARGS = \
/p:AndroidSupportedHostJitAbis=$(call join-with,:,$(ALL_HOST_ABIS)) \
/p:AndroidSupportedTargetAotAbis=$(call join-with,:,$(ALL_AOT_ABIS))

CONFIGURATIONS ?= Debug Release

.PHONY: leeroy jenkins leeroy-all opentk-jcw framework-assemblies
Copy link
Member

Choose a reason for hiding this comment

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

The build failed because the leeroy rule was building both configurations before and Jenkins uses jenkins rule, which depends on leeroy.

So we need to either let jenkins rule to run leeroy twice or change the Jenkins configuration to run make jenkins ... twice.

Now the Jenkins build ran make in this order:

+ make prepare-deps V=1 'MSBUILD_ARGS=/p:AutoProvision=True /p:AutoProvisionUsesSudo=True /p:IgnoreMaxMonoVersion=False'
+ make prepare jenkins V=1 'MSBUILD_ARGS=/p:AutoProvision=True /p:AutoProvisionUsesSudo=True /p:IgnoreMaxMonoVersion=False'
+ make all-tests V=1
+ make all-tests CONFIGURATION=Release V=1

So the tests in the Release configuration were ran without making prepare and all in Release configuration first.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we'll need to update Jenkins to run make leeroy twice, once per configuration value.

@radekdoulik: Thank you, I hadn't considered that problem. Doh!

@jonpryor
Copy link
Member

I've updated the PR builder so that it explicitly builds for both configurations separately.

Hopefully things will now work.

@jonpryor jonpryor merged commit ae093bf into dotnet:master Oct 25, 2018
luhenry
luhenry commented Oct 25, 2018
View reviewed changes
build-tools/scripts/Packaging.mk
_sl="$(ZIP_OUTPUT_BASENAME)/bin/$(CONFIGURATION)/lib/xamarin.android/xbuild/.__sys_links.txt"; \
if [ ! -f "$$_sl" ]; then continue; fi; \
for f in `cat $$_sl` ; do \
echo "$(ZIP_OUTPUT_BASENAME)/bin/$CONFIGURATION/lib/xamarin.android/xbuild/$$f" >> "$$_exclude_list"; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a typo here: $CONFIGURATION vs $(CONFIGURATION).

luhenry
luhenry commented Oct 25, 2018
View reviewed changes
Makefile
(cd $(call GetPath,JavaInterop) && make prepare CONFIGURATION=$(conf) JI_MAX_JDK=8) && \
(cd $(call GetPath,JavaInterop) && make bin/Build$(conf)/JdkInfo.props CONFIGURATION=$(conf) JI_MAX_JDK=8) && ) \
true
(cd external/xamarin-android-tools && make prepare CONFIGURATION=$(CONFIGURATION))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove these CONFIGURATION=$(CONFIGURATION).

luhenry
luhenry commented Oct 25, 2018
View reviewed changes
build-tools/scripts/BuildEverything.mk
$(call MSBUILD_BINLOG,leeroy-all,$(_SLN_BUILD),$(conf)) $(SOLUTION) /p:Configuration=$(conf) $(_MSBUILD_ARGS) && \
$(call CREATE_THIRD_PARTY_NOTICES,$(conf),bin/$(conf)/lib/xamarin.android/ThirdPartyNotices.txt,$(THIRD_PARTY_NOTICE_LICENSE_TYPE),True,False) && ) \
true
$(call MSBUILD_BINLOG,leeroy-all,$(_SLN_BUILD),$(CONFIGURATION)) $(SOLUTION) /p:Configuration=$(CONFIGURATION) $(_MSBUILD_ARGS) && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could simplify MSBUILD_BINLOG to not take $(CONFIGURATION) as a parameter, and simply use it in the template directly.

luhenry
luhenry commented Oct 25, 2018
View reviewed changes
build-tools/scripts/BuildEverything.mk
$(call CREATE_THIRD_PARTY_NOTICES,$(conf),bin/$(conf)/lib/xamarin.android/ThirdPartyNotices.txt,$(THIRD_PARTY_NOTICE_LICENSE_TYPE),True,False) && ) \
true
$(call MSBUILD_BINLOG,leeroy-all,$(_SLN_BUILD),$(CONFIGURATION)) $(SOLUTION) /p:Configuration=$(CONFIGURATION) $(_MSBUILD_ARGS) && \
$(call CREATE_THIRD_PARTY_NOTICES,$(CONFIGURATION),bin/$(CONFIGURATION)/lib/xamarin.android/ThirdPartyNotices.txt,$(THIRD_PARTY_NOTICE_LICENSE_TYPE),True,False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as https://github.com/xamarin/xamarin-android/pull/2325/files#r228338679.

@luhenry
Copy link
Contributor Author

luhenry commented Oct 25, 2018

In https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder/configure, you still have make create-vsix CONFIGURATIONS=Debug || true, shouldn't it be CONFIGURATION=Debug instead?

@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonpryor jonpryor jonpryor left review comments

@radekdoulik radekdoulik radekdoulik left review comments

Assignees
No one assigned
Labels
full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luhenry @jonpryor @radekdoulik