Add IBC support to managed build #10180
Conversation
This reverts commit 2754769.
|
@janvorli @adiaaida PTAL |
|
Two questions: where does this work assume ibcmerge will be installed (on the path? Some known directory?)? And do we have a version of ibcmerge that runs on linux to make the build.sh changes necessary? I understand putting in the support, but if we can't actually run ibc on linux, we should probably make ibcinstrument fail early/be ignored on linux. |
|
Also, can you include a sample project.json in the tests directory (maybe?) that can be copied into optdata/project.json so one doesn't have to be generated on the fly? |
|
@adiaaida ibcmerge is installed through BuildTools, and is available from within the context of msbuild. For CI, if we need to expose it to non-MSBuild based tools, the easiest thing I can think of is adding a new msbuild target so that you could read the output of, say, ibcmerge does not currently work on Linux; the work is planned, but for now it requires the desktop framework. We don't want Re sample.json: great idea, I'll add one. |
|
Ok, so if we're just calling build.cmd, it knows where to find it, assuming it's been installed through BuildTools |
|
@dpodder @dagood How is this and dotnet/corefx#17053 connected? I would expect for CoreCLR to consume IBCMerge it would need a similar set of changes to get all the necessary pieces restored. It doesn't automatically happen as part of consuming BuildTools unless it is correctly configured. |
|
@adiaaida Yes, that's correct. |
|
@weshaggard Good point, the three |
Depending on environment variables is almost always a bad idea as there are various reliability, discoverability, and security concerns with doing so. I would suggest either we pass them as "extra parameters" that just flow through to msbuild when we call build. I wouldn't plumb a ton of extra arguments into build.cmd/sh as there are already way too many IMO. |
|
@weshaggard @dpodder About relation to dotnet/corefx#17053: in CoreFX, only the build defs needed changes (and a CoreFX-specific Tools-Override update). There aren't any similar changes in CoreCLR because the defs aren't checked in here, but the "live" build definitions will need to be changed in the same way. (cc @wtgodbe) The |
|
@adiaaida test script added, PTAL. |
tests/scripts/optdata/bootstrap.py
Outdated
|
|
||
| def get_optdata_version(tool): | ||
| """Returns the version string specified in project.json for the given tool.""" | ||
| package_name = 'optimization.{0}.CoreCLR'.format(tool) |
There was a problem hiding this comment.
I don't really have a preference on this, but in other places, we've done this as
package_name = 'optimization.%s.CoreCLR' % (tool)
Likewise below
package_name = 'optimization.%s-%s.%s.CoreCLR' % (get_buildos(), arch, tool)
There was a problem hiding this comment.
Sounds good, I like consistency. I'll change it.
| @@ -0,0 +1,82 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Does this script basically just copy the project.json file into the appropriate directory in src/.nuget?
There was a problem hiding this comment.
That's one small part of it--if that was all I would have probably just written cmd/sh scripts. The bigger part is building up and printing the path(s) where optdata (PGO and/or IBC) needs to get copied for it to be consumed. This is harder than the project.json itself, and figuring it out was much easier in python than a shell script.
tests/scripts/optdata/bootstrap.py
Outdated
| return 'Windows_NT' | ||
| else: | ||
| sysname = os.uname()[0] | ||
| return 'OSX' if sysname.casefold() == 'Darwin'.casefold() else sysname |
There was a problem hiding this comment.
Can we just use sysname.lower() here? I know casefold is more aggressive than lower, but since all we are comparing to is a lowercase 'Darwin', I don't think we need that aggressive of a a lower, and casefold doesn't work in Python2 without importing extra libraries. If possible, I'd like this script to be as compatible with both Python2 and Python3 as possible since the main dependency in CoreCLR is on Python2.
There was a problem hiding this comment.
Yes, good catch; I didn't realize casefold() required extra packages on python2. I'll just use lower().
tests/scripts/optdata/bootstrap.py
Outdated
| for tool in TOOL_LIST: | ||
| for arch in ARCH_LIST: | ||
| optdata_dir = get_optdata_dir(tool, arch) | ||
| print(" * Copy {a} {t} files into: {d}".format(a=arch, t=tool, d=optdata_dir)) |
There was a problem hiding this comment.
What is actually happening here? Is this just a suggestion to the user where to copy their data to?
There was a problem hiding this comment.
Yes, and this is what is nontrivial--see my reply to the comment earlier.
| argparse.ArgumentParser(description=__doc__).parse_args() | ||
|
|
||
| SCRIPT_ROOT = path.dirname(path.realpath(__file__)) | ||
| REPO_ROOT = path.realpath(path.join(SCRIPT_ROOT, '..', '..', '..')) |
There was a problem hiding this comment.
I'd be more comfortable with the user being required to pass the repo_root, on the off chance that this file gets moved, or something about the repo structure changes. I doubt it would, but it's something to guard against.
There was a problem hiding this comment.
Hmm, good point. How about if I added some assertions like make sure REPO_ROOT\src\.nuget exists where we think it should? I also would like to keep the script easy to use, and counting the number of ..s needed is even more error-prone IMO.
* add assertion to make sure REPO_ROOT is correct * Use lower() instead of casefold() for simple case * Use `"%s" % (str)` interpolation sytnax for consistency
This lets them get called when running build or sync
| "valueType": "property", | ||
| "values": [], | ||
| "defaultValue": "" | ||
| }, |
There was a problem hiding this comment.
I'm not opposed to this if it makes things easier for you, but I haven't been updating/using config.jsons for new features so I don't accrue maintenance costs. The plan is to (somehow) make these common properties shared: dotnet/buildtools#1085.
There was a problem hiding this comment.
Thanks - I really like the idea of inheriting shared properties such as these from a common source. From the issue though it looks like this support isn't there yet? I'd like to move forward with the current implementation for now. We should be able to remove these entries from the local config.json later once they're implemented in a shared config, right?
There was a problem hiding this comment.
That's correct: no support, and we can remove them later.
My point is just that I tend to avoid adding them in the first place (using the -- /p:Foo=bar syntax instead) to avoid the extra maintenance cost. I'm fine leaving these in the PR, just a note.
There was a problem hiding this comment.
Ah, I see what you mean. I originally tried the -- /p:Foo=bar syntax too, but that turned out to be too fragile; there can only be one --, and it makes the arguments even more sensitive to ordering. One of the early CI failures was caused by this (it was trying to pass -rebuild to run.exe on the command line, after I had already fed it -- followed by msbuild args). I'd much rather keep it in its current form until the shared configs are ready.
|
This looks good to me. |
# Conflicts: # build.cmd # build.sh
| @@ -342,6 +354,24 @@ | |||
| "values": [], | |||
| "defaultValue": "" | |||
| }, | |||
| "OptionalToolSource": { | |||
There was a problem hiding this comment.
Do you actually need these here? Do they work with if you pass them after "--"? I ask because I don't see them being passed through the scripts.
There was a problem hiding this comment.
These are the same as the EnableProfileGuidedOptimization and OptimizationDataDir properties; the only difference is they are secrets, so they will need to be passed in from CI/VSTS or from users manually. VSTS support requires changes to not-checked-in pipeline definitions. CI support is coming in later PR(s). I want to go ahead with this PR before the CI changes are ready so that I can unblock manual IBC builds, which other work is dependent upon.
They can be passed directly to msbuild after --, but as I also noted above, that is much more fragile. No other parameters currently used in CI rely on --, which I think is a good thing. I'd really much rather add these settings to config.json for now, and remove them together with the non-secret ones once they're available in a shared config.
There was a problem hiding this comment.
Sounds good.
As an assign I really think the amount of argument parsing that is happening in these build scripts is way out of control which is at least part of the trouble with the extra parameter fragility.
|
Thanks for the detailed feedback @adiaaida @dagood @weshaggard! |
Add a new build switch, `ibcinstrument`, that adds `/Tuning` on the `crossgen` command line when building System.Private.CoreLib and its peers. Automatically consume IBC optdata during builds when these conditions are met: 1. `ibcinstrument` is *not* passed to the build, 2. optdata is available 3. ibcmerge is available Note that `optdata` will not yet be restored with this change; once packages for master are made available, a new package reference will still need to be added. This PR attempts to unblock manually testing IBC, as well as surrounding CI/infrastructure work. To help produce an IBC-optimized build using manually generated profile data, run the newly added `tests/scripts/optdata/bootstrap.py` script. It will configure the build to consume IBC data from a path automatically, and print out that path where profile data can be copied.
Add a new build switch,
ibcinstrument, that adds/Tuningon thecrossgencommand line when building System.Private.CoreLib and its peers. Automatically consume IBC optdata during builds when these conditions are met:ibcinstrumentis not passed to the build,Note that
optdatawill not yet be restored with this change; once packages for master are made available, a new package reference will still need to be added. This PR attempts to unblock manually testing IBC, as well as surrounding CI/infrastructure work.