dotnet / source-build

A repository to track efforts to produce a source tarball of the .NET Core SDK and all its components
MIT License
266 stars 132 forks source link

Unify defaults for logging and binlog creation when building the VMR #3966

Closed ViktorHofer closed 6 months ago

ViktorHofer commented 9 months ago

Feedback from @MichaelSimons from the Windows VMR PR.

I think we should consider adding a timestamp to the binlog and log files just as we do in the main build script for a better UX.

The arcade defaults when building an arcade-ified repository are the following:

Should the VMR repository follow those defaults? If not, should binlogs and log files contain timestamps?

cc @mmitche

Size: S

dotnet-issue-labeler[bot] commented 9 months ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

mmitche commented 9 months ago

I think I lean towards using the same defaults that Arcade uses. But, the VMR repo is a bit more special. However, if you're not working on infrastructure related changes, what value does the binlog add? Aren't you normally just looking at the compile errors for guidance on what to do next?

ViktorHofer commented 9 months ago

I'm also for using the Arcade defaults which is no binlogs and no logs locally (and not using the minimal log feature from the VMR). In CI we should always create binlogs and IMO use the minimal log feature. So, not really any changes aside from disabling logs locally.

MichaelSimons commented 9 months ago

Sharing my perspective, Yes the default behavior is attractive yet building source-build is a little different in complexity (often caused by integration). From a practical development perspective, source-build devs always want to have binlogs enabled. Because of that, it feels wrong to not tune the defaults to meet how developers are building.

ViktorHofer commented 9 months ago

Right now in main when building the VMR without source-only, binlogs and logs aren't created. Binlogs are only created if someone specifies the -bl flag or when running in CI.

When building source-only in the VMR, binlogs are always created.

If that's the desired experience, we can just close this issue then.

MichaelSimons commented 9 months ago

@mthalman, @ellahathaway, @NikolaMilosavljevic - please share your input.

mthalman commented 9 months ago

My default position on things like this is that the VMR should have the same defaults as the product repos. Is that standardized?

I agree that, for source build, binlogs are essential. If it's not already, we should make that the default mode for source build across all the repos.

ellahathaway commented 9 months ago

Binlogs are essential when source building. This should be the default mode across all repos. I'm in favor of producing Build.binlog and sourcebuild.binlog when the -sb flag is passed across all repos.

As far as the timestamp, I lean towards adding a timestamp to the binlogs when they're enabled as it aids in the developer workflow and eliminates unnecessary confusion about when the binlog was created.

At the same time, I agree that it makes sense for the VMR to have the same defaults as the product repos. Is there the option to update the arcade infra to add a timestamp to the binlog when the -bl switch is used? Then the VMR can follow this particular default.

ViktorHofer commented 8 months ago

Here's my take on this from the product side perspective.

We want our builds to be as fast as possible. The times that we hit msbuild issues are neglectable in comparison to the times that we face compiler errors or other toolset issues for which binlogs aren't helpful. We always want binlogs created as part of CI builds that happen on a remote, inaccessible machine as it might not be clear where the error is coming from. Those expectations are nicely met by Arcade's binlog defaults.

For CI, we don't need timestamps in the binlogs as the binlog format itself already records timestamps. In the case of flakiness we sometimes rerun a leg which then creates a new attempt in AzDO. For that, we already have to record the attempt number in the published artifact payload otherwise we would encounter clashes for any uploaded artifact.

Locally, we don't need/want timestamps encoded in the binlog's name either, as the creation of binlogs is opt-in. FWIW this behavior isn't controllable anyways when devs use the /bl flag which uses the default msbuild.binlog name and places it in the current working directory. I myself more often use /bl than the -bl flag.

I think it makes sense for the VMR / orchestrator to follow these defaults so that dotnet/dotnet doesn't artificially feel different to the individual repositories in terms of build entry-points (parameters and actions).

mmitche commented 8 months ago

I generally agree with Viktor here. When doing infra changes, I'll pretty much always pass -bl. But for normal operation, I would not expect a dev to do so. Certainly I am not asking for binlogs whenever I build arcade.

MichaelSimons commented 8 months ago

In an effort to unify around a standard set of behavior I am alright with changing the -bl behavior for the source-build dev experience. IMO this should however be treated as lower priority in the grand scheme of delivering the UB project in .NET 9.0