Skip to content

The new app / lib templates should help prevent checking in msbuild binlog files #14160

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

Open
jonthysell opened this issue Dec 4, 2024 · 5 comments · May be fixed by #14621
Open

The new app / lib templates should help prevent checking in msbuild binlog files #14160

jonthysell opened this issue Dec 4, 2024 · 5 comments · May be fixed by #14621
Labels
Area: App Template Area: CLI Area: Library Template enhancement good first issue: easy good first issue New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Workstream: Developer Experience Support the RNW developer inner-loop.
Milestone

Comments

@jonthysell
Copy link
Contributor

Summary

  1. The init-windows for the new templates should inject *.binlog into the project root .gitignore if the ignore file exists (and the line doesn't exist)
  2. (Optional): We shouldn't dump the binlog to the root folder, maybe we could put it somewhere else (maybe the windows folder, where it's easier for us to gitignore it) and then output the path to the log file at the end of the command.

Motivation

Right now, if you run run-windows --logging you'll get an msbuild.binlog file in the root of your project file (where you run run-windows 99% of the time). This file risks being checked in accidentally.

Basic Example

No response

Open Questions

No response

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Dec 4, 2024
@chrisglein
Copy link
Member

Related: #5937
Issue there was the binlog getting dropped in the root and having different rules for what the gitignore could do. Moving where it got dropped would help.

@chrisglein chrisglein added Area: App Template New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Area: Library Template Workstream: New Arch Soft Launch Soft launch the new architecture in 0.76. and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Dec 9, 2024
@chrisglein chrisglein added this to the Backlog milestone Dec 9, 2024
@jonthysell
Copy link
Contributor Author

Changes for this will most likely be in vnext\templates\templateUtils.js to be used by the templateConfig.js files in the cpp-lib and cpp-app templates.

@jonthysell for more help/guidance.

@frankcalise
Copy link
Contributor

@jonthysell Looking at helping with this issue.

  1. Seems straight forward enough, would you do something like that inside the cli at initWindows.ts?
  2. Going this route would avoid having to do 1 at all, do we just want to default buildLogDirectory to the windows dir, then update the vnext ignore templates?

I suppose we would want to add *.err and *.wrn in addition to *.binlog. Thoughts?

Image

@TatianaKapos TatianaKapos added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Apr 24, 2025
@chrisglein chrisglein added Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Apr 24, 2025
@jonthysell
Copy link
Contributor Author

@jonthysell Looking at helping with this issue.

  1. Seems straight forward enough, would you do something like that inside the cli at initWindows.ts?
  2. Going this route would avoid having to do 1 at all, do we just want to default buildLogDirectory to the windows dir, then update the vnext ignore templates?

I suppose we would want to add *.err and *.wrn in addition to *.binlog. Thoughts?

Image

I agree if we just did 2 that would give us what we need and not pollute the project's main folder with build output. It's interesting to me that if you don't specify a folder with --buildLogDirectory, we only keep the binlog file and then put the err and wrn files in the temp folder.

I think then, the fix looks something like this in msbuildttools.ts:

  1. Update the logPrefix to default to the windows folder (rather than the tmp folder) if buildLogDirectory is not specified:
    const logPrefix = path.join(
    buildLogDirectory || os.tmpdir(),
    `msbuild_${process.pid}_${target}`,
    );
  2. Remove the localBinLog (I don't see why we need that at all) and update the const binlog to
const binlog = `:${logPrefix}.binlog`;

here:

const localBinLog = target === 'build' ? '' : ':deploy.binlog';
const binlog = buildLogDirectory ? `:${logPrefix}.binlog` : localBinLog;

Then we just need to add *.binlog, *.err, and *.wrn to the template _gitignore files at vnext/templates/cpp-app/windows/_gitignore and vnext/templates/cpp-lib/windows/_gitignore

@frankcalise frankcalise linked a pull request Apr 25, 2025 that will close this issue
@frankcalise
Copy link
Contributor

@jonthysell thanks for the guidance! I have a draft PR up, I still want to test it locally better.

I was reading the Contributing to the CLI docs, but I didn't quite grok the proper way to test my local CLI changes just yet.

@chrisglein chrisglein removed the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: App Template Area: CLI Area: Library Template enhancement good first issue: easy good first issue New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Workstream: Developer Experience Support the RNW developer inner-loop.
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

4 participants