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.

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

Revert "Remove source-build workaround for sourcelink submodule issue" #39356

Merged
merged 1 commit into from Jan 6, 2022

Conversation

MichaelSimons
Copy link
Member

This reverts commit 9b049bd (#39324) as the change broke the internal builds. See https://github.com/dotnet/aspnetcore-internal/issues/3979#issuecomment-1006801036.

@MichaelSimons MichaelSimons requested a review from a team as a code owner January 6, 2022 18:46
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 6, 2022
@MichaelSimons MichaelSimons enabled auto-merge (squash) January 6, 2022 18:54
@dougbu
Copy link
Member

dougbu commented Jan 6, 2022

@dotnet/aspnet-build should we merge this pre-emptively to get builds working again❔ In particular, public builds aren't failing so the CI validation here proves nothing.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 6, 2022

No objections here

@wtgodbe wtgodbe disabled auto-merge January 6, 2022 19:13
@wtgodbe wtgodbe merged commit 00f7e1f into dotnet:main Jan 6, 2022
@ghost ghost added this to the 7.0-preview1 milestone Jan 6, 2022
@dougbu
Copy link
Member

dougbu commented Jan 6, 2022

Latest official build worked❕

Now, main question is why public source-build jobs worked but internal ones did not. Any ideas @MichaelSimons

@MichaelSimons MichaelSimons deleted the revert-PR-39324 branch January 6, 2022 20:40
@MichaelSimons
Copy link
Member Author

I don't have any idea. The only difference that I see is the Agent pool. The source build jobs run in a container which I would expect us to be isolated from most machine config changes.

@ghost
Copy link

ghost commented Jan 6, 2022

Hi @MichaelSimons. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Jan 6, 2022

Hmm, there are a couple of differences in our YAML between public and internal, including a few from Arcade-powered Source Build itself. Eg. $(_PublishArgs) is empty in public builds and the following internally:

  - name: _PublishArgs
    value: /p:Publish=true
           /p:GenerateChecksums=true
           /p:DotNetPublishBlobFeedKey=$(dotnetfeed-storage-access-key-1)
           /p:DotNetPublishBlobFeedUrl=https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json
           /p:DotNetPublishToBlobFeed=$(_DotNetPublishToBlobFeed)
           /p:DotNetPublishUsingPipelines=$(_PublishUsingPipelines)
           /p:DotNetArtifactsCategory=$(_DotNetArtifactsCategory)

We use that variable in the source-build.yml job: buildScript: './eng/build.sh $(_PublishArgs) --no-build-repo-tasks'. But, I don't understand how those msbuild properties could cause the submodule failures we saw…

@@ -13,6 +14,19 @@
WorkingDirectory="$(InnerSourceBuildRepoRoot)" />
</Target>

<!--
Init submodules - temporarary workaround for https://github.com/dotnet/sourcelink/pull/653
Copy link
Contributor

Choose a reason for hiding this comment

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

dotnet/sourcelink#653 was fixed by @tmat in 2018. perhaps file a new issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are tracking the backport of this patch with dotnet/source-build#2708.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants