Add blname parameter to build scripts#3859
Conversation
|
Changes along these lines have been discussed at length in the past. It's quite often that changes like this are simply to work around an awkward or unintentional design and just lead to a more complicated underlying system. I'm not saying that is the case here, just saying that we should be intentional with changes like this. |
|
I agree, opened the PR in order to discuss here and so that the proposed changes are clear in here. Compared to the PR opened in the past, is adding an extra argument instead of changing the current switch which would be a breaking change to do so, as if it is not a switch, you can't do: |
|
I added my 2 cents here: #1978 (comment) |
|
cc: @markwilkie |
|
@safern This PR is adding a new feature to support a workaround implemented in CoreFX rather then stating what the actual problem is. The discussion of the actual problem is in a closed issue. :( Perhaps we could first find out if we actually need the workaround and what is the best approach to address the root problem. |
Well, not necessary because of the workaround, even without the need to restore the internal tools, I think we would like to still do the restore step in a separate phase than the build step because of what I stated in the other discussion:
|
|
Then this should be stated as the reasoning in the PR. |
|
Also that's a workaround for binlog viewer not being able to present larger binlog better. Perhaps that can be improved instead. |
I agree, what I don't understand is, if we're providing tooling, why repos can't have the extensibility or options in this tooling to configure the naming of the produced logs? For example, msbuild does gives you those options, of course it has a default, but you can always specify your own naming, so I don't understand why this feature has been such a discussion and a blocker. I understand that we should be conservative and understand why features are needed to keep sanity, but to me and apparently other people like @AdamYoblick and @zsd4yr brought this up, it is not only me who would like this feature. I could just do uggly stuff like passing an extra |
|
@safern The issue is not primarily with naming the binlog file differently, although it breaks consistency across repos. The issue is with the reason why it is requested. You want to name the file differently to split the build. The build was designed to run all (or a subset of) the build phases in one go, so that the CI build can be easily reproduced on a dev machine the same way in all repos. If you start splitting the build into different phases, with different parameters, in different repos then then the contributor to a particular repo needs to search and understand your yaml files to see how to run that particular phase of the build in your repo. The same with artifacts - if they are named differently in different repos then I need to figure out what I'm looking for first for each repo I'm dealing with. Besides, the reasons why running build script multiple times with different parameters have been requested in past were just workarounds for other issues that should have been addressed instead. |
| [string] $projects, | ||
| [string][Alias('v')]$verbosity = "minimal", | ||
| [string] $msbuildEngine = $null, | ||
| [string] $blname = $null, |
| InitializeCustomToolset | ||
|
|
||
| $bl = if ($binaryLog) { "/bl:" + (Join-Path $LogDir "Build.binlog") } else { "" } | ||
| $bl = if ($binaryLog) { |
There was a problem hiding this comment.
Since this is not a single-line if expression anymore, it would be more readable to use an if statement.
| echo " --configuration <value> Build configuration: 'Debug' or 'Release' (short: -c)" | ||
| echo " --verbosity <value> Msbuild verbosity: q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic] (short: -v)" | ||
| echo " --binaryLog Create MSBuild binary log (short: -bl)" | ||
| echo " --blname <value> Create MSBuild binary log with the given name" |
|
I appreciate what the build was originally designed to do, but I agree with Santiago on this.
If customers want to split the steps, they are going to do it regardless. As they should be able to.
Giving customers more options (especially ones they request) in a supported, standardized way seems like it would generate less support headaches than having customers spin their own logic every time (which may differ from team to team).
For winforms specifically, we wanted to split the steps because we didn’t like one monolithic build step. It wasn’t a workaround for other problems, the single build step WAS the problem in our opinion.
The same argument can be made for the new collection of post-build phases. Why are they all different phases, why not just one phase with one big step?
The answer to that is the same answer to why some teams want to separate restore/build/test/pack/publish. Because they are different things. :)
|
|
Separating steps is something very important for the CI council, especially the restore step. Many of the repositories do an up-front restore in a separate steps to fail early and produce the right telemetry. I don't have any concerns with adding the -blName option besides the growing complexity of our build scripts. |
Are you saying that repositories that do not do this in a separate YAML step report wrong telemetry? |
Not necessarily. The default restore step is flagged automatically but anything custom needs to be flagged additionally. Just took a look at our report, we hardly have "unknown" failures. |
|
Then I don't understand the reasoning in this sentence:
|
|
Separating restore from build or any other level of separation isn't really (IMO), high on the priority list for CI council. CI council recognizes that different repos have different challenges that may necessitate the form of that repo's build. CI council is more interested in understanding metrics about the parts that makeup a build rather than how that build is split. ie, if restore is its own step or part of the build step, isn't a particular concern; but our engineering telemetry should give us the metrics to understand our builds regardless of how they are split. |
|
I want to ping on this thread to agree on a final decision, I think all of us have made our arguments broadly and there are some specs and conclusions here... To summarize, the reason for the push back on this feature is because we don't want to encourage people to split or "separate" steps of the build, because that is not how arcade was designed for. We completely understand that and we agree on the purpose for that, however there are some scenarios where people might decide they want to split steps for various reasons, like reading logs on azure devops, having different logs to download and analyze separately, or others like isolate restore issues from build issues. I think at the end people that work on those repos will do it if they think is a better experience to maintain their builds, or if it is necessary to do so in order to workaround limitations of azure devops for logging or for example in corefx, to avoid having multiple dotnet sdks in the agent, given an sdk is needed in the path to restore internal tools. I think this feature, rather than encouraging people to do that, will avoid people from hard coding paths, passing a double parameter on CI builds (because they will eventually pass down a I just want to know if this is going to be accepted given the facts I just summarized in this comment or not. If it won't be taken, it's fine, we're not blocked, but it is not ideal to do workarounds that way. |
I'm not confident this is true. Let's chat offline @safern as I want to understand this better. |
I think part of it is because more customization options means support and servicing becomes harder. So now you have to check the yaml to see where the logs are for the specific step that ran, etc... I get that. But if teams are going to do it anyway with /bl: AND hardcode the path to the artifacts dir...supporting it officially seems like the lesser of two evils. Just my thoughts on it 😄 |
|
Let me suggest a solution to separation of build and restore that we can implement in Arcade. This will also demonstrate why we don't want The purpose of We wouldn't be able to make this change if repos explicitly set the binlog name and expect it to find it under that name. I'd rather see us addressing the root of the problem then adding a new API that allows some repos to workaround the problem. |
I think it's a mistake to assume the purpose of being able to pass the log name is just to separate build from restore. The purpose is to allow customers to split steps however they see fit. In the winforms case, we wanted to split compile, unit test, integration tests, and sign/pack/publish. Azdo output looks much cleaner to us. Logs are smaller and easier to navigate.
In our opinion, running tests should not be lumped together with compiling, as they are two independent actions. Arcade already follows the same pattern with ToolsetRestore.binlog, this is just a logical expansion of that train of thought. |
|
I still want to chat with @safern about this so I understand better. |
|
A practical argument for allowing splitting the build into separate steps is binlog size. This build https://dev.azure.com/dnceng/internal/_build/results?buildId=422550&view=results produced a 553 MB binlog that the log viewer can't open. Four 125 MB log files would be usable and easier to analyse. With the repo consolidation happening this will become even more of a problem. Splitting Restore, Build, Test, and Publish into separate steps would make these binlog files much smaller. |
|
@alexperovich I think it would make sense to split the binlogs to workaround KirillOsenkov/MSBuildStructuredLog#235 as I pointed out in #3859 (comment). It would be even better to actually fix KirillOsenkov/MSBuildStructuredLog#235. |
I don't think we will get 4x125 MB though. It would be useful to try it out first. My guess is that build will still be the biggest one followed by restore. |
|
I'm mostly w/ @tmat on this one. As long as we've got build steps that do a fair amount in a single But, I disagree that separating I would generally prefer we put some work into improving one or more of
|
Could you try to experiment with what I suggested in #3859 (comment)? Just locally call |
🆗 I'll report back l8r |
|
In 'release/3.0' branch of aspnet/AspNetCore:
I did a |
|
@dougbu Thanks for the numbers. Seems like it'd make sense to call MSBuild separately for restore and the rest of the build. Agreed? |
Agreed; I was surprised the Note the |
|
@nguerrera FYI |
|
Is this still needed? |
|
It's not technically needed, because teams have a workaround. But the workaround involves hardcoding the path to the artifacts directory in the yaml. I feel like an officially supported way that's a little less hacky would be better. But if people disagree, and if this workaround is acceptable from a support/servicing perspective, then ok 😄 |
|
If we agree on taking this, I'll update the PR to address feedback and fix conflicts. |
|
I finally was able to talk w/ @safern, and I have much better context. The crux of this issue (in my view) are the implications to our value proposition of keeping things consistent and "simple". In this case, what seems to be emerging is a difference in opinion on how to deal with build "steps"... (namely, inside or outside of build) @safern has indicated that we should know what the dotnet/runtime Arcade requirements are in a few weeks. So before we close on this, I'd like to wait a little longer and discuss this issue in context with those requirements, with the goal of addressing the underlying "cause" here and not just putting a band aid on something. Thanks all for your continued engagement, and more soon. |
|
@markwilkie @safern just pinging to see if the the dotnet/runtime have Arcade requirements clarified in the past few weeks and if we know if we're interested in taking this or not. |
|
Good ping @jonfortescue. Anything related to his emerge @safern ? |
|
We workaround it by using the MSBuild |
10 participants
Currently, when the
-blflag or-ciflag is passed, there is no way to override the binary log name that is produced. The only way to do so, is to add an extra /bl:PathToBinlog, at the end of the script, where in the yaml files or elsewhere you need to hard code the path to the log dir, in order for arcade's step to publish logs to artifacts to be able to find it.With this change, if -blname is passed, a binlog is created in the log dir, with the given name.
cc: @ViktorHofer