Skip to content

[release/6.0] Pass /norestart to Hosting Bundle nested bundles #47545

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

Merged
merged 1 commit into from
Apr 4, 2023
Merged

[release/6.0] Pass /norestart to Hosting Bundle nested bundles #47545

merged 1 commit into from
Apr 4, 2023

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Apr 3, 2023

Backport of #47542 to release/6.0

/cc @wtgodbe

Pass /norestart to Hosting Bundle nested bundles

Ensure that when /norestart is passed on the command line to the hosting bundle, the user sees the expected behavior (no restart)

Description

WiX doesn't pass command line args to nested bundles/packages, since the nested package could be any arbitrary .exe. This means when that today, when the user passes /norestart to the hosting bundle in a repair or uninstall command, they'll still be prompted to restart. Always passing /norestart to these commands will give us the behavior that users would expect (/norestart = no restart, no /norestart = restart, prompted from the hosting bundle itself).

Fixes #47544

Customer Impact

Fulfills request from the Office team

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

These /norestart args will only have any effect when the user passes /norestart to the hosting bundle. The risk is if there's a scenario I haven't thought of.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 3, 2023
@ghost ghost added this to the 6.0.x milestone Apr 3, 2023
@ghost
Copy link

ghost commented Apr 3, 2023

Hi @wtgodbe. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@wtgodbe wtgodbe requested review from joeloff and a team April 3, 2023 18:03
@ghost
Copy link

ghost commented Apr 3, 2023

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@wtgodbe wtgodbe added the Servicing-consider Shiproom approval is required for the issue label Apr 3, 2023
@ghost
Copy link

ghost commented Apr 3, 2023

Hi @wtgodbe. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@wtgodbe
Copy link
Member Author

wtgodbe commented Apr 4, 2023

@BrennanConroy there's a consistent SignalR test failure on MacOS 11, could you take a look? It doesn't appear to be related to this change.

at RunTests.ProcessUtil.RunAsync(String filename, String arguments, String workingDirectory, String dumpDirectoryPath, Boolean throwOnError, IDictionary2 environmentVariables, Action1 outputDataReceived, Action1 errorDataReceived, Action1 onStart, CancellationToken cancellationToken) in /private/tmp/helix/working/B821096E/w/B1DD094D/e/RunTests/ProcessUtil.cs:line 210
at RunTests.ProcessUtil.RunAsync(String filename, String arguments, String workingDirectory, String dumpDirectoryPath, Boolean throwOnError, IDictionary2 environmentVariables, Action1 outputDataReceived, Action1 errorDataReceived, Action1 onStart, CancellationToken cancellationToken) in /private/tmp/helix/working/B821096E/w/B1DD094D/e/RunTests/ProcessUtil.cs:line 210
at RunTests.TestRunner.CheckTestDiscoveryAsync() in /private/tmp/helix/working/B821096E/w/B1DD094D/e/RunTests/TestRunner.cs:line 190
2023-04-03T21:39:33.5056860Z RunTest stopping due to test discovery failure.

@BrennanConroy
Copy link
Member

You didn't copy the actual error:
'/tmp/helix/working/B821096E/p/dotnet-cli/dotnet vstest Microsoft.AspNetCore.SignalR.Common.Tests.dll -lt' completed with exit code '137'
Not sure there's anything I can help with here... it's failing on test discovery.

@wtgodbe
Copy link
Member Author

wtgodbe commented Apr 4, 2023

Not sure there's anything I can help with here... it's failing on test discovery.

You're not aware of anything the SignalR.Common tests do that's "special"? Only that work item is failing, and it's happened on each of the 4 CI runs in this PR.

I don't see how it could be related to this hosting bundle change, but it didn't happen in #47527 so it's either transient or something changed on the agents. I'll try re-running while I see if I can dig anything up.

@BrennanConroy
Copy link
Member

Looking at a recent successful and this PR shows a difference in macos version.
Passing : 11.6.6
Failing: 11.6.8

Looking at the passed test output from https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-aspnetcore-refs-pull-47527-merge-e18dac652a42493182/Microsoft.AspNetCore.SignalR.Common.Tests--net6.0/1/console.ff2721b3.log?helixlogtype=result the only oddity I can think of is items like:
Microsoft.AspNetCore.SignalR.Common.Tests.Internal.Protocol.Utf8BufferTextWriterTests.WriteUnicodeStringAndCharsWithVaryingSegmentSizes(singleChar: 'い', segmentSize: 14) where it has a weird character in it.

@wtgodbe
Copy link
Member Author

wtgodbe commented Apr 4, 2023

I asked in First Responders if it's a known issue. Worst case scenario we could just bump to MacOs 12 like 7.0:

<HelixAvailableTargetQueue Include="OSX.1200.Amd64.Open" Platform="OSX" />
.

@leecow leecow modified the milestones: 6.0.x, 6.0.17 Apr 4, 2023
@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

Hi @wtgodbe. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@wtgodbe
Copy link
Member Author

wtgodbe commented Apr 4, 2023

Huh, guess it's transient

@BrennanConroy
Copy link
Member

That run ran an macos-11.6.6

@wtgodbe
Copy link
Member Author

wtgodbe commented Apr 4, 2023

I'll see if I can get a run of #47560 on 11.6.8

@wtgodbe wtgodbe merged commit 4a47904 into dotnet:release/6.0 Apr 4, 2023
@wtgodbe wtgodbe deleted the wtgodbe/NoRestart6 branch April 4, 2023 21:01
@ghost ghost modified the milestone: 6.0.17 Apr 4, 2023
@rbhanda rbhanda removed this from the 6.0.17 milestone Jun 1, 2023
@rbhanda rbhanda added this to the 6.0.18 milestone Jun 1, 2023
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 Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants