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

Release build workflow #797

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

dtchepak
Copy link
Member

Added a release build on tag.

Example: https://github.com/dtchepak/NSubstitute/actions/runs/8864632563 (with minor tweak to build-on-push for testing).

@304NotModified
Copy link
Contributor

Example: https://github.com/dtchepak/NSubstitute/actions/runs/8864632563 (with minor tweak to build-on-push for testing).

Package looks good, expect this is still 5.1.x

image

Copy link
Contributor

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

LGTM, needs only the NuGet push and correct version number.

If you like to test that, you could do that by pushing to https://int.nugettest.org/ AFAIK

.github/workflows/release_build.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,44 @@
name: Release build
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the name "Package and documentation" would describe this flow better.

(could be also Packages and documentation)

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to "Build release packages and documentation".

@dtchepak dtchepak force-pushed the release-ci branch 2 times, most recently from b1f48e4 to 184ab9d Compare April 29, 2024 10:51
- Rename workflow
- Split artifact upload into packages and docs
@dtchepak
Copy link
Member Author

Example: https://github.com/dtchepak/NSubstitute/actions/runs/8864632563 (with minor tweak to build-on-push for testing).

Package looks good, expect this is still 5.1.x

Oh nice catch! Just double-checked the code and I think it's all good, it's just I hadn't pushed the 5.2 tag to my fork (so it used the previous 5.1 tag).

Have made the suggested changes. Sample: https://github.com/dtchepak/NSubstitute/actions/runs/8877342178

Once it's merged i'll push 5.2.1 and release that.

@304NotModified
Copy link
Contributor

304NotModified commented Apr 29, 2024

Once it's merged i'll push 5.2.1 and release that.

are there any (code) changes between 5.2.1 and 5.2? Otherwise I suggest to re-define the 5.2 tag

bundler-cache: true

- name: Build package and docs
run: dotnet run --project 'build/build.fsproj' -- -t All
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to use dotnet pack for packaging instead of custom tool and use tool only for documentation

- name: Pack
  run: dotnet pack -c Release -p:Version=${{ inputs.Version }} -o "bin/Release/NSubstitute" NSubstitute\NSubstitute.csproj

- name: Validate and build documentation
  run: dotnet run build/build.fsproj -- -t Documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

also as I see we have following fragment in csproj

  <PropertyGroup Condition="'$(CI)' == 'True'">
    <!--Deterministic Build and Source Link settings -->
    <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
    <EmbedUntrackedSources>true</EmbedUntrackedSources>
    <IncludeSymbols>true</IncludeSymbols>
    <SymbolPackageFormat>snupkg</SymbolPackageFormat>
  </PropertyGroup>

maybe also make sense to add /p:CI=True or remove this condition from PropertyGroup (probably better to remove)
but I'm not an expert in snupkg and don't know how it works

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't do ContinuousIntegrationBuild locally and those settings are needed for deterministic packages/source link. Therefore the check on CI

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't disagree with this, but do you have an outline of the pros and cons of switching?

The current FAKE-based build sorts out our versioning so I don't think it's as straightforward as switching directly (given constraint that I'd like to make version based on repo contents rather than an input).

Maybe can defer until larger build changes are done? (as per #797 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, 100% it could be changed later.

My private opinion:
dotent cli is a default way to interact with .NET projects like build\restore\pack\publish. From my point of view, make sense to use default approach for default problems. Just no need to reinvent the wheel. Fake and fsharp is just additional complexity.

.github/workflows/release_build.yml Show resolved Hide resolved
.github/workflows/release_build.yml Show resolved Hide resolved
.github/workflows/release_build.yml Show resolved Hide resolved
with:
dotnet-version: |
6.0.x
7.0.x
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove .NET 7 sdk from this list

Copy link
Contributor

Choose a reason for hiding this comment

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

.NET 7 is still used here.

<TargetFrameworks>net8.0;net7.0;net6.0;net462</TargetFrameworks>

Removing this is a breaking change I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

no, we don't need to run test for packing. we use for tests another pipeline

Copy link
Contributor

Choose a reason for hiding this comment

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

Good one :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Current targets have test as dependency so will still run. Can do this if we switch to dotnet pack

Copy link
Contributor

Choose a reason for hiding this comment

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

Current targets have test as dependency so will still run. Can do this if we switch to dotnet pack

Maybe that's indeed better, but IMO is not that important. I don't mind the .NET 7 here

@dtchepak
Copy link
Member Author

@alexandrnikitin @zvirja : do either of you have time to take a quick look at this? 🙇

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.

None yet

3 participants