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

Revert SDK update #32785

Merged
merged 4 commits into from
May 19, 2021
Merged

Revert SDK update #32785

merged 4 commits into from
May 19, 2021

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented May 17, 2021

Reverts the SDK update from #32581 to unblock internal builds

@captainsafia should I undo any other changes from that PR?

@wtgodbe wtgodbe requested review from captainsafia and a team May 17, 2021 20:01
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 17, 2021
@wtgodbe wtgodbe requested a review from a team as a code owner May 17, 2021 20:27
@wtgodbe
Copy link
Member Author

wtgodbe commented May 18, 2021

@captainsafia @pranavkm getting lots of errors like:

d:\workspace_work\1\s\src\Components\test\testassets\BasicTestApp\BindCasesComponent.razor(22,75): error RZ9991: The attribute names could not be inferred from bind attribute 'bind-value'. Bind attributes should be of the form 'bind' or 'bind-value' along with their corresponding optional parameters like 'bind-value:event', 'bind:format' etc. [D:\workspace_work\1\s\src\Components\test\testassets\BasicTestApp\BasicTestApp.csproj]

What's the best way to disable these failing components?

@pranavkm
Copy link
Contributor

Those are all of Blazor's e2e tests. Skipping them would leave us with a large test coverage gap, I don't think we would want to consider that. Is the failure persistent?

@wtgodbe
Copy link
Member Author

wtgodbe commented May 18, 2021

Those are all of Blazor's e2e tests. Skipping them would leave us with a large test coverage gap, I don't think we would want to consider that. Is the failure persistent?

Yes, it's a build failure. I think because I removed the ProjectReference & they're using assets from Wasm.Performance.TestApp?

@pranavkm
Copy link
Contributor

No, it's a different RazorSDK issue. @captainsafia there's a tracking issue for this, no?

@captainsafia
Copy link
Member

Yes, the tracking issue is #31023. The SDK that is being reverted contains the fix.

We've primarily since this issue happen locally (@HaoK has seen this on his Playwright migration branch) and not on CI so surprising to see it here. It might be the case that the changes in the build made in this branch have exasperated the issue.

Unfortunately, I don't think we can readily disable the BasicTestApp here. One thing we can try to do is disable node reuse in the build (there's been success with that locally) but not sure if that will fix it in CI.

@wtgodbe
Copy link
Member Author

wtgodbe commented May 18, 2021

The failure doesn't happen without the non-sdk related changes in this PR. How flaky is Wasm.Performance.TestApp? Maybe the least flaky thing is to let it keep building. We'll have new machines to use next wednesday

@captainsafia
Copy link
Member

The failure doesn't happen without the non-sdk related changes in this PR.

Can you clarify what you mean here?

How flaky is Wasm.Performance.TestApp?

I think it was causing failures around 20% of the time before we resolved the issue.

@wtgodbe
Copy link
Member Author

wtgodbe commented May 18, 2021

Can you clarify what you mean here?

If this PR only contained the change in global.json, CI would be green - the failures are caused by the other changes in this PR.

@dougbu
Copy link
Member

dougbu commented May 19, 2021

@wtgodbe BasicTestApp is still being built. Why❔

/fyi I resolved the merge conflict and cleaned up the PoliCheck violation introduced in 8b52d1c

@dougbu
Copy link
Member

dougbu commented May 19, 2021

Will need to backport this PR to release/6.0-preview5 now that I branched.

@dougbu dougbu merged commit 57b9c13 into main May 19, 2021
@dougbu dougbu deleted the wtgodbe/sdkDown branch May 19, 2021 22:26
@ghost ghost added this to the 6.0-preview5 milestone May 19, 2021
@dougbu
Copy link
Member

dougbu commented May 20, 2021

/backport to release/6.0-preview5

1 similar comment
@dougbu
Copy link
Member

dougbu commented May 20, 2021

/backport to release/6.0-preview5

@github-actions
Copy link
Contributor

Started backporting to release/6.0-preview5: https://github.com/dotnet/aspnetcore/actions/runs/859333855

@ghost
Copy link

ghost commented May 20, 2021

Hi @github-actions[bot]. 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 May 20, 2021

That didn't work

{"message":"Error: @dougbu is not a repo collaborator, backporting is not allowed.","postToGitHub":true}

Need to fix this since I am a collaborator.

But, @wtgodbe first priority is to get this into release/6.0-preview5. Let me know if you get to this tomorrow before I do.

dougbu pushed a commit that referenced this pull request May 20, 2021
@wtgodbe @pranavkm @dougbu
[preview5] Revert SDK update (#32785)
8087909 
* Revert SDK update
* Disable build of failing proj
* Quarantine 2 tests
* Fixup

- backport of 57b9c13

Co-authored-by: Pranav K <[email protected]>
@dougbu
Copy link
Member

dougbu commented May 20, 2021

first priority is to get this into release/6.0-preview5

Done in #32859

@dougbu dougbu modified the milestones: 6.0-preview5, 6.0-preview6 May 20, 2021
@wtgodbe
Copy link
Member Author

wtgodbe commented May 20, 2021

Thanks @dougbu @pranavkm!

@wtgodbe
Copy link
Member Author

wtgodbe commented May 20, 2021

@akoeplinger any idea what the issue would be causing Doug's issue above? #32785 (comment)

@wtgodbe
Copy link
Member Author

wtgodbe commented May 20, 2021

We need dotnet/runtime@be17f71 - will open a PR after standup

dougbu added a commit that referenced this pull request May 20, 2021
@dougbu @wtgodbe @pranavkm
[preview5] Revert SDK update (#32785) (#32859)
a205f49 
* Revert SDK update
* Disable build of failing proj
* Quarantine 2 tests
* Fixup

- backport of 57b9c13

Co-authored-by: William Godbe <[email protected]>
Co-authored-by: Pranav K <[email protected]>
dougbu added a commit that referenced this pull request May 20, 2021
@dougbu @wtgodbe
@pranavkm @dotnet-maestro
[automated] Merge branch 'release/6.0-preview5' => 'main' (#32885)
e897454 
- no-op merge

* Update dependencies from https://github.com/dotnet/efcore build 20210519.2 (#32855)

[release/6.0-preview5] Update dependencies from dotnet/efcore

* Update dependencies from https://github.com/dotnet/runtime build 20210520.4 (#32882)

[release/6.0-preview5] Update dependencies from dotnet/runtime

* [preview5] Revert SDK update (#32785) (#32859)
  * Revert SDK update
  * Disable build of failing proj
  * Quarantine 2 tests
  * Fixup

  - backport of 57b9c13

Co-authored-by: William Godbe <[email protected]>
Co-authored-by: Pranav K <[email protected]>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Doug Bunting <[email protected]>
Co-authored-by: William Godbe <[email protected]>
Co-authored-by: Pranav K <[email protected]>
dougbu added a commit that referenced this pull request May 25, 2021
@dougbu
Revert "Revert SDK update (#32785)"
88a959b 
- take us back to the 6.0.100-preview.5.21264.3 SDK

This reverts commit 57b9c13.
@dougbu dougbu mentioned this pull request May 25, 2021
Merged
dougbu added a commit that referenced this pull request May 26, 2021
@dougbu
Merge pull request #33018 from dotnet/dougbu/revert.revert.sdk
2c8fdd8 
Bump the SDK back up again
- publish in a separate step
  - Publishing.props executes the `_GetPackageVersionInfo` target in Microsoft.AspNetCore.App.Runtime.csproj
  - that target currently fails when using a new SDK and desktop `msbuild`
- revert "Revert SDK update (#32785)"
  - take us back to the 6.0.100-preview.5.21264.3 SDK
  - this reverts commit 57b9c13.
- reword a comment
- add Components.Migration.E2ETests to solution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm pranavkm approved these changes

@captainsafia captainsafia captainsafia approved these changes

@dougbu dougbu dougbu approved these changes

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
6.0-preview6
Development

Successfully merging this pull request may close these issues.
4 participants
@wtgodbe @pranavkm @captainsafia @dougbu