Skip to content

Create copilot-instructions.md #115917

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 9 commits into from
May 26, 2025
Merged

Create copilot-instructions.md #115917

merged 9 commits into from
May 26, 2025

Conversation

CarnaViire
Copy link
Member

@CarnaViire CarnaViire commented May 23, 2025

First iteration on the instructions.

Testing on #115826.

Despite the remaining environment limitations, the instructions themselves are working.
See e.g. https://github.com/dotnet/runtime/actions/runs/15200538664/job/42753739967#step:9:1582

Note: I've also tried adding the clr build as a setup step, but unfortunately it didn't stick 🥲

@CarnaViire
Copy link
Member Author

CarnaViire commented May 23, 2025

Based on the logs: it used the instructions to build clr runtime
Based on the same logs: ...but the build takes too long 🙃 (it takes 16 min and the time limit is 2 min)

@CarnaViire CarnaViire marked this pull request as ready for review May 23, 2025 02:29
@Copilot Copilot AI review requested due to automatic review settings May 23, 2025 02:29
@CarnaViire CarnaViire requested review from jeffhandley and a team as code owners May 23, 2025 02:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a new GitHub Actions workflow step to install dependencies for building the runtime and adds the initial copilot instructions documentation for the project. The changes include:

  • Adding a step in the workflow file to install required system dependencies.
  • Creating a documentation file with detailed build and test instructions for dotnet/runtime.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/copilot-setup-steps.yml Added a dependency installation step using apt-get commands
.github/copilot-instructions.md Introduced instructions for building and testing the runtime
Comments suppressed due to low confidence (1)

.github/copilot-instructions.md:5

  • There is a typo in the word 'trailling'; it should be 'trailing'.
* Don't leave trailling whitespaces.

@CarnaViire CarnaViire changed the title [WIP] Create copilot-instructions.md Create copilot-instructions.md May 23, 2025
@agocke
Copy link
Member

agocke commented May 23, 2025

Based on the same logs: ...but the build takes too long 🙃 (it takes 16 min and the time limit is 2 min)

This is unattainable. We are nowhere near being able to build any substantial portion of the repo in 2 min.

@danmoseley
Copy link
Member

Cc @Chuxel for any additional thoughts

@danmoseley
Copy link
Member

We likely need to open the firewall too

@Chuxel
Copy link

Chuxel commented May 23, 2025

Re: the setup instructions - generally less is more. For example, rather than spelling out some code formatting rules, you could tell it to run a linter before submitting. We've definitely seen being explicit with the linter to use help and increase the liklihood it does it. The number of liters out there is probably less of an issue than other languages (e.g. Python has many), but we know it helps for Go too. That said, if this is giving you the results you are looking for - awesome! Really interesting to see ... also cc @shibbas both for a real-world example and any other tips.

Based on the logs: it used the instructions to build clr runtime Based on the same logs: ...but the build takes too long 🙃 (it takes 16 min and the time limit is 2 min)

@ellismg released an update that should resolve the time out in now - let us know if not.

We likely need to open the firewall too

The good news is we realized the public AzDO package should be open by default since it's a well-known, trusted, public source So that's resolved. You should be able to ignore cdn.fwupd.org unless you literally need Linux firmware updates. There are a few of these that are getting caught that are things like OS updaters... and its good they're getting caught, but we likely will filter these known things that don't cause harm out of notices since we know they can be confusing.

@CarnaViire
Copy link
Member Author

@Chuxel

Re: the setup instructions - generally less is more. For example, rather than spelling out some code formatting rules, you could tell it to run a linter before submitting.

The main goal was to teach the agent how to build and test, and how to know what to build and test. TBH at this point the reason I added the code style ones (based on ASP.NET Core's version) is just to have some additional verification these instructions are actually read. 😅

During the investigation in #115826, at some point it felt like it was using some echo of the comment #115826 (comment) rather than copilot-instructions.md I added. And as the comment grew older, I guess that comment was either left out of the context, or severely downweighted, and the agent temporarily forgot how to use build.sh 🙃 But by now I see it actually referencing the instructions from the file. Maybe it was confused by the similarity (or some kind of conflict) of the two how-tos.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I think we can adjust as necessary. If it seems like it's struggling, we can add/remove things. The tricky thing with the runtime repo is that everyone has their own workflow because they want to build as little as possible to get to their scenario.

@CarnaViire
Copy link
Member Author

I ended up providing the pre-built clr+libs to the agent through the setup steps (45b25c6), and that helped tremendously.

Before, copilot was still struggling with missing bits and pieces (varying from run to run), and the occasional aggressive build timeouts (but there were cases of successful long builds, too).

Successful run with the latest instructions and setup: link

copilot: Now let's build and run the tests to verify the fix:
<...>
    command: cd /home/runner/work/runtime/runtime && ./build.sh -subset libs -rc release
<...>
    Build succeeded.
        0 Warning(s)
        0 Error(s)
<...>

copilot: Now let's run the specific test to see if our fix resolves the issue:
<...>
    command: cd /home/runner/work/runtime/runtime && dotnet test src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/Microsoft.Extensions.Http.Tests.csproj -v n
<...>
   Test Run Successful.
    Total tests: 185
         Passed: 185
     Total time: 9.5574 Seconds
<...>

copilot: Great! The test is now passing. Let's commit this change

I am tempted to :shipit: since I've got an approval from @agocke
But if someone could pls take a look and sign off the latest addition, that would be great 😄
/cc @ericstj @agocke @danmoseley

@CarnaViire
Copy link
Member Author

/ba-g copilot infra-only change

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

A few small suggestions. This looks pretty good. Thank you for putting together, @CarnaViire

@CarnaViire
Copy link
Member Author

/ba-g copilot infra-only change

@CarnaViire
Copy link
Member Author

Addressed all remaining nits; ready for merge.
Follow-ups tracked in #115946.
Time to :shipit:

@CarnaViire CarnaViire merged commit f85ec36 into main May 26, 2025
15 of 16 checks passed
@hoyosjs hoyosjs deleted the CarnaViire-patch-1 branch May 27, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants