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

Stop making src modifications to every repo's eng/common/tools.sh #3171

Closed
Tracked by #3664
MichaelSimons opened this issue Dec 9, 2022 · 8 comments · Fixed by dotnet/installer#17664
Closed
Tracked by #3664
Assignees
Labels
area-infra Source-build infrastructure and reporting

Comments

@MichaelSimons
Copy link
Member

When building the VMR, there are several checked in files that are being modified. This yields a poor UX as these must be undone before you can build cleanly again. These edits will also become a nuisance in .NET 9 when the VMR can be used to checkin changes.

One type of file being modified is each repo's repo's eng/common/tools.sh. This file is updated to workaround #2307

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@MichaelSimons MichaelSimons moved this to 8.0 Preview 1 in .NET Source Build Dec 9, 2022
@MichaelSimons MichaelSimons added area-infra Source-build infrastructure and reporting and removed untriaged labels Dec 15, 2022
@MichaelSimons MichaelSimons moved this from 8.0 Preview 1 to 8.0 Backlog in .NET Source Build Feb 9, 2023
@MichaelSimons MichaelSimons moved this from Needs Review to 8.0 Backlog in .NET Source Build Jun 8, 2023
@MichaelSimons MichaelSimons moved this from 8.0 Backlog to 8.0 Preview 7 in .NET Source Build Jun 22, 2023
@MichaelSimons MichaelSimons moved this from 8.0 Preview 7 to 8.0 Backlog in .NET Source Build Jun 22, 2023
@MichaelSimons MichaelSimons moved this from 8.0 Backlog to 9.0 Lull in .NET Source Build Jul 20, 2023
@NikolaMilosavljevic
Copy link
Member

@MichaelSimons @mmitche - source-build is modifying eng/common/tools.sh to update the following line: https://github.com/dotnet/arcade/blob/f5d1610c0943aefc60193d26c43736b2c61a8ed6/eng/common/tools.sh#L512 to return $exit_code instead of 0.

The following 2 environment variables are available during Bash script execution, and can be used to push the VMR change upstream, which would add some source-build conditioned code:

DotNetBuildFromSource=true
DotNetBuildFromSourceFlavor=Product

The alternative would be to dig deeper and understand if we could either: 1) drop source-build change altogether, or 2) modify Arcade's code to return $exit_code. Option 2 would run against the following comment and likely introduce extra logging noise - it's unknown if noise is all that would be added, or if some CI log-parsing code would fail. Here's the existing comment in Arcade: https://github.com/dotnet/arcade/blob/f5d1610c0943aefc60193d26c43736b2c61a8ed6/eng/common/tools.sh#L509-L512

@mmitche
Copy link
Member

mmitche commented Sep 19, 2023

Does the history indicate why source-build changed to update this to return $exit_code?

@NikolaMilosavljevic
Copy link
Member

Does the history indicate why source-build changed to update this to return $exit_code?

It seems that this source-build workaround can simply be removed - #2307 (comment)

cc @MichaelSimons

@MichaelSimons
Copy link
Member Author

It seems that this source-build workaround can simply be removed - #2307 (comment)

This would need to be validated. The exit code was not preserved and therefore would not fail the build. SB runs with the CI flag set.

@NikolaMilosavljevic
Copy link
Member

Besides workaround for #2307, we also update eng/common/tools.sh to add another workaround for repos that are not using the latest Arcade: https://github.com/dotnet/installer/blob/1f0982b7f469aa0d693d1ad6744a7fde5263ef1b/src/SourceBuild/content/repo-projects/Directory.Build.targets#L67-L73

There are two repos that require this today: command-line-api and xdt.

@MichaelSimons
Copy link
Member Author

to add another workaround for repos that are not using the latest Arcade:

In .NET 9.0, I am hopeful that the VMR will force every repo to be on the same arcade version. Ideally there would only be one copy of the eng/common directory in the VMR.

Shorter term, could we allow control via an ENV?

@NikolaMilosavljevic
Copy link
Member

to add another workaround for repos that are not using the latest Arcade:

In .NET 9.0, I am hopeful that the VMR will force every repo to be on the same arcade version. Ideally there would only be one copy of the eng/common directory in the VMR.

Shorter term, could we allow control via an ENV?

We could utilize env.vars to condition this code. I presume the proposal is to make the necessary, conditioned, changes in eng/common/tools.sh of the affected repos, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infra Source-build infrastructure and reporting
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants