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

Clarify ASan test running instructions #5240

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

Conversation

davidmrdavid
Copy link
Member

@davidmrdavid davidmrdavid commented Jan 16, 2025

This is an exceedingly minor contribution, so feel free to disregard it.

When running the ASan'ized tests, I could have used a reminder that the python test script resides within the .\out\<arch> folder. The preceeding cmake commands should run in the repo's root, so it's easy to forget changing your directory when running the python <pathToTestScript>/stl-lit.py command.

This PR adds a reminder that the call to stl-lit.py should be performed from out\x64\ , to make this clearer.

Please feel free to dismiss this, it's a subjective contribution. Thanks!

@davidmrdavid davidmrdavid requested a review from a team as a code owner January 16, 2025 02:50
@davidmrdavid davidmrdavid changed the title [Proposed] Add subtle reminder ASan tests also need to run inside the \out\<arch directory [Proposed] Add subtle reminder ASan tests also need to run inside the \out\<arch> directory Jan 16, 2025
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@davidmrdavid davidmrdavid changed the title [Proposed] Add subtle reminder ASan tests also need to run inside the \out\<arch> directory Modify ASan test running instructions to have them run from the repo's root Jan 16, 2025
@StephanTLavavej StephanTLavavej added the documentation Related to documentation or comments label Jan 16, 2025
@StephanTLavavej StephanTLavavej self-assigned this Jan 16, 2025
@davidmrdavid
Copy link
Member Author

naive question: should I go ahead and squash-and-merge is there a procedure to this?

@CaseyCarter
Copy link
Member

CaseyCarter commented Jan 16, 2025

naive question: should I go ahead and squash-and-merge is there a procedure to this?

We hand-port all changes into the MSVC repo and merge in both places "simultaneously" to minimize divergence between the internal and external copies of the STL. Yes, this is a bit of a PITA. We generally merge a batch of PRs every few days to amortize the annoyance of the port and the cost of the internal CI. The process is documented in https://github.com/microsoft/STL/wiki/Checklist-For-Merging-A-Pull-Request.

TLDR: No, we'll squash and merge it later. Your work is done here once the PR reaches the "Ready to Merge" state. Right now it's at "Final Review" because I want to see if STL is bothered by the ASan instructions using the top of the repo as working directory instead of using out/x64 like the non-ASan instructions do.

@StephanTLavavej
Copy link
Member

I want to see if STL is bothered by the ASan instructions using the top of the repo as working directory instead of using out/x64 like the non-ASan instructions do.

Yeah, this really bothers me. It's unnecessarily different from the normal test instructions which begin by saying:

These examples assume that your current directory is C:\Dev\STL\out\x64.

I'll push a change.

@StephanTLavavej StephanTLavavej changed the title Modify ASan test running instructions to have them run from the repo's root Clarify ASan test running instructions Jan 17, 2025
@StephanTLavavej
Copy link
Member

Change pushed (and PR title and description updated).

Sorry to be so nitpicky, but I believe that following a consistent pattern is important. Running the tests from out\<arch> has a couple of benefits: (1) if you run a different arch (commonly x86), you only need to alter your pushd command, and (2) if you create any log or temp files while investigating tests, they'll be safely contained within the out tree instead of polluting the repo root.

@StephanTLavavej StephanTLavavej removed their assignment Jan 17, 2025
@davidmrdavid
Copy link
Member Author

no problem at all, it's good for me to absorb these documentation values for future PRs, so I appreciate them being laid out explicitly. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments
Projects
Status: Ready To Merge
Development

Successfully merging this pull request may close these issues.

4 participants