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

Move away from shared project #8376

Merged
merged 12 commits into from
Oct 11, 2024
Merged

Move away from shared project #8376

merged 12 commits into from
Oct 11, 2024

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Oct 9, 2024

This is an alternative approach to share code between the two clients. This is a tad more inline with the dotnet CLI as we no longer depend on legacy projects.

The impetus for this change was to improve cold compilation times.

My local jetBrains Rider kept locking up trying to do the initial compile.

  • Moves us over to <ArtifactsPath> (set to $root\.artifacts like other .NET projects)
  • Removes a leftover <OutputPath> for certain projects (this was tripping up deps.json resolving HARD).
  • No longer use nuget package locking
    • nuget resolves to lowest anyway and we never float dependencies
    • no other .NET repository at Elastic does this.

@flobernd
Copy link
Member

flobernd commented Oct 10, 2024

This looks great ❤️ Thanks a lot for creating this PR.

I will need some time to review in detail, but already have one comment regarding the locked restore:

Would keeping the package lock have any other noticable drawback? I usually like that approach as it e.g. allows for easy package caching in CI (based on a hash of all package.lock.json files). This speeds up build times on the slow GitHub workers quite a bit:

uses: 'actions/cache@v4'
with:
path: '~/.nuget/packages'
key: '${{ runner.os }}-nuget-${{ inputs.flavor }}-${{ hashFiles(env.LOCKFILE_PATTERN) }}'
restore-keys: '${{ runner.os }}-nuget-${{ inputs.flavor }}-'

To mitigate some of the "developer experience issues" like having to use --force-evaluate, my usual approach is to guard the condition like this:

<RestorePackagesWithLockFile>true</RestorePackagesWithLockFile>
<RestoreLockedMode Condition="'$(ContinuousIntegrationBuild)' == 'true'" />

The CI passes that argument whenever the project must be built or packaged:

-p:ContinuousIntegrationBuild=true

What do you think?

@Mpdreamz
Copy link
Member Author

We simply cache by [cf]proj files. Slightly higher cache busting rate but in practice not a big issue :)

https://github.com/elastic/apm-agent-dotnet/blob/3ffe9b4f24f2e3cfdc5e82c1a42e346d64d36fb9/.github/workflows/bootstrap/action.yml#L39

@flobernd
Copy link
Member

No super strong opinion here, but given that the package.lock.json approach has no real drawbacks (AFAIK), I think it's the more robust approach. These files always represent the final package graph.

Hashing just [cf]proj won't capture changes in includes for these files as well as Directory.Packages.props, Directory.Build.props, Directory.Build.targets, etc.

Directory.Build.props for example is currently used to include the DotNet.ReproducibleBuilds package.

@flobernd flobernd added 8.x Relates to 8.x client version Area: Build Category: Enhancement labels Oct 11, 2024
@flobernd flobernd merged commit 8a074ef into main Oct 11, 2024
22 of 24 checks passed
@flobernd flobernd deleted the fix/pass branch October 11, 2024 13:35
Copy link
Contributor

The backport to serverless failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-serverless serverless
# Navigate to the new working tree
cd .worktrees/backport-serverless
# Create a new branch
git switch --create backport-8376-to-serverless
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 8a074efe45e91cf6db83cbc1780b70081b3e5494
# Push it to GitHub
git push --set-upstream origin backport-8376-to-serverless
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-serverless

Then, create a pull request where the base branch is serverless and the compare/head branch is backport-8376-to-serverless.

Copy link
Contributor

The backport to 8.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.x 8.x
# Navigate to the new working tree
cd .worktrees/backport-8.x
# Create a new branch
git switch --create backport-8376-to-8.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 8a074efe45e91cf6db83cbc1780b70081b3e5494
# Push it to GitHub
git push --set-upstream origin backport-8376-to-8.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.x

Then, create a pull request where the base branch is 8.x and the compare/head branch is backport-8376-to-8.x.

flobernd pushed a commit that referenced this pull request Oct 11, 2024
flobernd pushed a commit that referenced this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants