Skip to content

Support having spaces in OfficialBuilder #677

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
May 21, 2025

Conversation

omajid
Copy link
Member

@omajid omajid commented May 21, 2025

Currently, running the following command fails:

$ ./build.sh --prep --source-build /p:OfficialBuilder='Red Hat'

Lots of scripts and projects assume that there are no spaces in OfficialBuilder (and other properties). Having spaces breaks in all sorts of places. This is a minimal fix to try and resolve that.

The primary change is to use bash arrays to store values (properties and arguments) to make sure they are quoted correctly when passed to other places.

@omajid omajid requested review from a team as code owners May 21, 2025 14:21
@omajid
Copy link
Member Author

omajid commented May 21, 2025

I am not sure what the code flow looks like these days. Please let me know if I should open PRs against the individual component repos instead.

@mthalman
Copy link
Member

The change to build.sh should only be made in the arcade repo. This is because it's within the eng/common directory. Edits within that directory should only be made in arcade and dependency flow will flow those changes to all the other repos, including the VMR.

@mthalman
Copy link
Member

The change to build.sh should only be made in the arcade repo.

By arcade repo, I mean https://github.com/dotnet/arcade, not the src/arcade directory in the VMR.

@ViktorHofer
Copy link
Member

ViktorHofer commented May 21, 2025

Sorry, @mthalman that I have to disagree here. We have a different policy:

  • Changes to Versions.props & Version.Details.xml must not be made inside the VMR (unless you really know what you're doing -> they don't sync back to the repo)
  • Changes to eng/common files must be made under src/arcade/eng/common only. The VMR repos build with those build scripts (except for the VMR orchestrator & SBRP which use the root eng/common ones).
  • Updating the .NET SDK or the Arcade.Sdk in VMR root's or src/arcade's global.json or updating the root eng/common files must only be done as part of a re-bootstrap PR (we have automation for that).

Based on that, @omajid please revert all the eng/common changes except for the one under src/arcade.

... and yes, we should document this. Also see https://github.com/dotnet/dotnet?tab=readme-ov-file#where-to-make-changes

@ellahathaway
Copy link
Member

  • Changes to eng/common files must be made under src/arcade/eng/common only. The VMR repos build with those build scripts (except for SBRP which uses the root eng/common ones).
  • Updating the .NET SDK or the Arcade.Sdk in VMR root's or src/arcade's global.json or updating the root eng/common files must only be done as part of a re-bootstrap PR (we have automation for that).

FYI - dotnet/source-build#5185 will help prevent changes to eng/common.

@omajid omajid force-pushed the official-build-id-spaces branch from 869d8a1 to 24322a7 Compare May 21, 2025 18:49
@omajid
Copy link
Member Author

omajid commented May 21, 2025

Thanks, @mthalman and @ViktorHofer ! I have reverted all eng/common changes except to src/arcade/eng/common. However, that means that building with /p:OfficialBuilder='Red Hat' breaks when building SBRP, since it uses the root eng/common without the fixes. I guess I have to wait for the re-bootstrap?

Currently, running the following command fails:

    $ ./build.sh --prep --source-build /p:OfficialBuilder='Red Hat'

Lots of scripts and projects assume that there are no spaces in
OfficialBuilder (and other properties). Having spaces breaks in all
sorts of places. This is a minimal fix to try and resolve that.

The primary change is to use bash arrays to store values (properties and
arguments) to make sure they are quoted correctly when passed to other
places.
@omajid omajid force-pushed the official-build-id-spaces branch from 24322a7 to b76efe1 Compare May 21, 2025 18:57
@mthalman
Copy link
Member

I guess I have to wait for the re-bootstrap?

That's correct.

@ViktorHofer
Copy link
Member

Yes. If we have a passing build tonight, I can do another re-bootstrap tomorrow. I want one for other reasons as well.

@ViktorHofer ViktorHofer enabled auto-merge (squash) May 21, 2025 20:50
@ViktorHofer ViktorHofer merged commit 8ef8cdd into dotnet:main May 21, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants