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

Adding a build script #33

Merged
merged 6 commits into from
May 24, 2018
Merged

Adding a build script #33

merged 6 commits into from
May 24, 2018

Conversation

jorive
Copy link
Member

@jorive jorive commented May 22, 2018

  • Added build.py (executable)
  • 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

- 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
@jorive
Copy link
Member Author

jorive commented May 22, 2018

This is the build script help output:

build.py --help
usage: build.py [-h] [-c CONFIGURATION] [-f [FRAMEWORK [FRAMEWORK ...]]] [-v]

Builds the CoreClr benchmarks.

optional arguments:
  -h, --help            show this help message and exit
  -c CONFIGURATION, --configuration CONFIGURATION
                        Configuration use for building the project (default
                        "Debug").
  -f [FRAMEWORK [FRAMEWORK ...]], --frameworks [FRAMEWORK [FRAMEWORK ...]]
                        Target frameworks to publish for (default all).
  -v, --verbose         Turns on verbosity (default "False")
``` #ByDesign

build.py Outdated
default='Debug',
choices=['debug', 'release'],
type=str.casefold,
help='Configuration use for building the project (default "Debug").',
Copy link
Member

@brianrob brianrob May 22, 2018

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

Copy link
Member Author

@jorive jorive May 22, 2018

Choose a reason for hiding this comment

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

Makes sense #Resolved

@brianrob
Copy link
Member

brianrob commented May 22, 2018

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

Copy link
Member

@brianrob brianrob left a 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. :)

@jorive
Copy link
Member Author

jorive commented May 22, 2018

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__))
Copy link
Contributor

@michellemcdaniel michellemcdaniel May 22, 2018

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:
Copy link
Contributor

@michellemcdaniel michellemcdaniel May 22, 2018

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

Copy link
Member Author

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)

Copy link
Contributor

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):
Copy link
Contributor

@michellemcdaniel michellemcdaniel May 22, 2018

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can do that.


In reply to: 190038807 [](ancestors = 190038807)

@@ -0,0 +1,190 @@
@REM This is a template script to run .NET Performance benchmarks with CoreRun.exe
Copy link
Contributor

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?

Copy link
Member Author

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)

Copy link
Contributor

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
@michellemcdaniel
Copy link
Contributor

What are the init.py files for? They are all blank

@jorive
Copy link
Member Author

jorive commented May 23, 2018

@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).
UPDATE: It seems that __init__.py files are not needed on python 3.3+
PEP 420 -- Implicit Namespace Packages
I will need to look more into it... Future PR?

raise FatalError("Unsupported python version.")

args = process_arguments()
configuration, frameworks, verbose = args
Copy link
Member

@DrewScoggins DrewScoggins May 23, 2018

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

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Member Author

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.

@jorive jorive merged commit 9a0b211 into dotnet:master May 24, 2018
@jorive jorive deleted the dev/buildpy branch May 24, 2018 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants