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

Deemphasize MSBUILDDEBUGENGINE in binlog doc #8712

Merged
merged 1 commit into from
May 1, 2023
Merged

Conversation

rainersigwald
Copy link
Member

@rainersigwald rainersigwald commented Apr 27, 2023

At least one person skimmed over the section we wanted to emphasize (-bl) and focused on MSBUILDDEBUGENGINE, sharing lower-fidelity logs that are harder to understand.

Remove the "Preferred way" callout--it's preferred in that section but not in general. Add a section header for command-line builds. Add some samples there.

At least one person skimmed over the section we wanted to emphasize (`-bl`) and focused on `MSBUILDDEBUGENGINE`, sharing lower-fidelity logs that are harder to understand.

Remove the "Preferred way" callout--it's preferred in that section but not in general. Add a section header for command-line builds. Add some samples there.
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -6,23 +6,37 @@ However, you should be aware what type of information is captured in the binary

⚠ NOTE: some build environments make secrets available using environment variables. Before sharing a binary log, make sure it does not expose API tokens or other important secrets.

## Capturing Binary Logs for command-line builds
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to add "preferred way" or "preferred way for command-line builds" to this header?

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems a bit much to me, but I pushed a change to try to both explain why VS is different and also emphasize the command-line option. Look ok?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit concerned someone might read that and think binlogs from VS aren't trustworthy at all. If something only reproduces from VS, I can tell it must be doing something different from a command line build...but what? How is it different? When does it build that the command line doesn't or vice versa? I think I'd be ok if I had some assurance that building from within VS isn't wrong or if I didn't see that at all, but I think this version raises more questions than it answers, personally.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@rainersigwald rainersigwald force-pushed the binlog-deemphasize-env branch from ce24e12 to 1754079 Compare April 28, 2023 21:29
@rainersigwald rainersigwald added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 28, 2023
@Forgind Forgind merged commit d95c8c3 into main May 1, 2023
@Forgind Forgind deleted the binlog-deemphasize-env branch May 1, 2023 20:38
Set `MSBUILDDEBUGENGINE` environment variable to `'1'` and (optionally) set `MSBUILDDEBUGPATH` to an existing destination folder to store the captured logs. Then start Visual Studio from the same shell to inherit the environment:

`cmd:`
```

```batch
> SET MSBUILDDEBUGENGINE=1
Copy link

Choose a reason for hiding this comment

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

@rainersigwald I see this has already been merged (and the changes look good!), but I wanted to provide feedback that using > in a sample block (both here and below) isn't ideal:

  • It's already in a batch/powershell block and formatted appropriately.
  • If someone copy/pastes it directly, then it doesn't work, because > is a redirection character.

(No need to create a new PR for this, but please keep in mind for future edits)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants