-
Notifications
You must be signed in to change notification settings - Fork 274
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
Adding a build script #33
Conversation
- Added build.py - Updated the `src/coreclr/common.props` file to suppress .NET Core Preview warnings - Added a script to run `src/coreclr` benchmarks against CoreCLr's CoreRun - Updated src/coreclr readme
This is the build script help output: build.py --help
|
build.py
Outdated
default='Debug', | ||
choices=['debug', 'release'], | ||
type=str.casefold, | ||
help='Configuration use for building the project (default "Debug").', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the default should probably be release since these are perf tests. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense #Resolved
Overall, looks good. In terms of strategy, how are we planning to get from a set of steps that have manual steps right now to something that we can automate? Also, are we planning to start these runs off by running against full SDKs? If so, we shouldn't have to patch anything - we just reference the packages via and should be good to go, right? #Resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as approved. I don't want to block your merge. :)
The plan is to run against the full SDK, but we also want to support running against previous stages of the .NET Core repo-pipeline (this is why I added the support to CoreClr's CoreRun). In reply to: 391099264 [](ancestors = 391099264) |
build.py
Outdated
def get_script_path() -> str: | ||
'''Gets this script directory.''' | ||
return sys.path[0] | ||
# return os.path.dirname(os.path.realpath(__file__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to remove this commented out line. #Resolved
build.py
Outdated
# return os.path.dirname(os.path.realpath(__file__)) | ||
|
||
|
||
def get_repo_root_path() -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we need both get_repo_root_path and get_script_path if one just calls the other? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the script under src/coreclr
, but Drew mentioned he could use it, so I moved it to the root. I think this way it is cleaner if we decide to move it under the scripts folder.
In reply to: 190031742 [](ancestors = 190031742)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Makes sense.
build.py
Outdated
) | ||
|
||
|
||
class DotNet(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move some of the classes into their own files? This script is super long with all the class definitions, and it makes it hard to see what is going on. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,190 @@ | |||
@REM This is a template script to run .NET Performance benchmarks with CoreRun.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to add a shell script version of this as well for linux platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we? I had this more as an example than something to be used for automation, so it's clear what needs to be done to run against the CoreClr build.
If we needed I rather convert it to python script. What do you think?
In reply to: 190042328 [](ancestors = 190042328)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think that if we have a way to run under corerun on windows, we should also for linux platforms. Converting to a python script would be good.
- Added missing annotations - Removed an extra coma on statement - Moved classes to its own file under the build module
What are the init.py files for? They are all blank |
@adiaaida Python needs them to treat the folder as a package, and at this moment, I'm not using them (you could have code there too). |
raise FatalError("Unsupported python version.") | ||
|
||
args = process_arguments() | ||
configuration, frameworks, verbose = args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should probably be verbosity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to add verbosity to the script for debuggability. This flag was to turn it on/off.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that he has a potential verbosity level option commented out, I feel like the bool of verbose on or off makes sense as verbose, rather than verbosity. Verbosity implies levels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the verbose setting that we pass to the publish and restore steps is always the default? Is this just for controlling the verbosity of the script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-v
is only for enable/disable the script verbosity.
src/coreclr/common.props
file to suppress .NET Core Preview warningssrc/coreclr
benchmarks against CoreCLr's CoreRun