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

Build only for Linux x64 on CI #7763

Closed
wants to merge 6 commits into from
Closed

Conversation

emlautarom1
Copy link
Contributor

Fixes #7762

Changes

  • Build only for linux-x64 on Github Workflows

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

Notes on testing

CI should be green without affecting other workflows.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

When running dotnet build we're currently generating native files for platforms that we don't care about like tvos-arm64 among several others. All of these files take considerable space which can cause issues due to disk usage on CI.

In an ideal world we could list exactly the platforms that we target but alas this does not work (see dotnet/sdk#42153 for an extremely similar case), which limits us to specifying at most one single platform.

@emlautarom1 emlautarom1 requested review from rubo and a team as code owners November 15, 2024 19:25
@emlautarom1 emlautarom1 force-pushed the fix/ci-single-platform branch from 306267e to a58d7cb Compare November 15, 2024 21:00
<PropertyGroup Condition="'$(CI)' == 'true'">
<!-- Required due to an issue with transitive dependencies of Colorful.Console -->
<!-- See: https://github.com/tomakita/Colorful.Console/pull/94 -->
<NoWarn>NU1605</NoWarn>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Colorful.Console does not officially support .NET 8 which causes issues when specifying runtimes. I'm not sure if the issue just does not show up in "normal" builds but is still there, or is it only due to the usage of specific runtimes.

Microsoft's docs are quite poor in this case and the proposed workaround just does not work: https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1605#example-3

Copy link
Contributor

Choose a reason for hiding this comment

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

Yet another reason to deprecate Nethermind.Cli.

@emlautarom1 emlautarom1 requested a review from alexb5dh November 15, 2024 21:08
Copy link
Contributor

@rubo rubo left a comment

Choose a reason for hiding this comment

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

Given that we no longer have disk space or package audit issues, I'd revert all the changes while keeping the added tests.

@emlautarom1 emlautarom1 force-pushed the fix/ci-single-platform branch from e8a20a8 to c3311e5 Compare November 19, 2024 20:49
@emlautarom1
Copy link
Contributor Author

@rubo how is it that we're no longer facing disk space issues? What did we do to fix this issue?

Copy link
Contributor

@rubo rubo left a comment

Choose a reason for hiding this comment

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

@rubo how is it that we're no longer facing disk space issues? What did we do to fix this issue?

If you wanna see how's that, revert your changes and watch workflows complete successfully. You need the latest master, though. It now purges the disk before starting, hence no storage issues.

@@ -28,4 +28,4 @@ jobs:
- name: Set up .NET
uses: actions/setup-dotnet@v4
- name: Build ${{ matrix.solution }}.sln
run: dotnet build src/Nethermind/${{ matrix.solution }}.sln -c ${{ matrix.config }}
run: dotnet build src/Nethermind/${{ matrix.solution }}.sln -c ${{ matrix.config }} -p:CI=true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary.

@@ -32,7 +32,7 @@ jobs:
uses: actions/setup-dotnet@6bd8b7f7774af54e05809fcc5431931b3eb1ddee #v4.0.1
- name: Build Nethermind
working-directory: src/Nethermind
run: dotnet build Nethermind.sln -c release
run: dotnet build Nethermind.sln -c release -p:CI=true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary.

@@ -81,7 +82,7 @@ jobs:
- name: ${{ matrix.project }}
id: test
run: |
dotnet test src/Nethermind/${{ matrix.project }} -c release \
dotnet test src/Nethermind/${{ matrix.project }} -c release -p:CI=true \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary.

@@ -147,7 +149,7 @@ jobs:
- name: ${{ matrix.project }}
id: test
run: |
dotnet test src/Nethermind/${{ matrix.project }} -c release \
dotnet test src/Nethermind/${{ matrix.project }} -c release -p:CI=true \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary.

Comment on lines +3 to +9
<PropertyGroup Condition="'$(CI)' == 'true'">
<!-- Required due to an issue with transitive dependencies of Colorful.Console -->
<!-- See: https://github.com/tomakita/Colorful.Console/pull/94 -->
<NoWarn>NU1605</NoWarn>
<RuntimeIdentifier>linux-x64</RuntimeIdentifier>
</PropertyGroup>

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary.

@emlautarom1 emlautarom1 mentioned this pull request Nov 20, 2024
14 tasks
@emlautarom1
Copy link
Contributor Author

Closed in favor of #7772. We might want to revisit this approach though given that it should speed up CI time since it generates less artifacts.

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.

Broken CI due to lack of space
3 participants