-
Notifications
You must be signed in to change notification settings - Fork 12
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
Null tests.142 #290
base: main
Are you sure you want to change the base?
Null tests.142 #290
Conversation
2078d23
to
3f04df9
Compare
update local branch for change in fre make command names
7b22d9c
to
c7c62d7
Compare
7859ff6
to
fe3b347
Compare
for the |
""" | ||
check if --verbose option works | ||
""" | ||
create_checkout_script.checkout_create(YAMLFILE,PLATFORM,TARGET,False,False, False,True) |
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.
@singhd789 @thomas-robinson perhaps create_checkout_script
should return
the full path to checkout.sh
? instead of just printing it to screen?
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.
Well, the whole path to the checkout script is printed (and it's made executable) - that way the user knows where it is and could also run it themselves if they want. What would be another benefit of returning it as well? Just curious
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.
this test + the otheres could become a simple:
compile_sh_path = create_checkout_script (<BLAH ARGS>)
assert Path(compile_sh_path).exists()
similarly, elsewhere in the code base, if create_checkout_script
is being used, that returned string object could be actually leveraged by other functions. If it's just print
, then there is no option to do so
check if --execute option works | ||
""" | ||
subprocess.run(["rm","-rf",f"{OUT}/fremake_canopy/test"]) | ||
create_checkout_script.checkout_create(YAMLFILE,PLATFORM,TARGET,False,2, True,False) |
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.
OK so i saw that what brought this PR's checks from red-x to green-check is the change of this functions argument from a boolean value to an integer 2
. This suggests to me that it's hard to track what these arguments are supposed to be when developing the code, so checkout_create
needs explicit keyword arguments, not positional.
this line should read like:
create_checkout_script.checkout_create(
yamlfile = YAMLFILE,
platform = PLATFORM,
target = TARGET,
no_parallel_checkout = False,
jobs = 2,
execute = True,
verbose = False )
target = "prod-openmp" | ||
|
||
# Set example yaml paths, input directory | ||
CWD = Path.cwd() |
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.
If you use the ci.gnu
platform with the TEST_BUILD_DIR
defined for the modelRoot (
modelRoot: ${TEST_BUILD_DIR}/fremake_canopy/test |
CWD
@@ -18,7 +18,9 @@ def checkout_create(yamlfile, platform, target, no_parallel_checkout, jobs, exec | |||
run = execute | |||
jobs = str(jobs) | |||
pcheck = no_parallel_checkout | |||
|
|||
|
|||
if jobs == 'False' and execute: |
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 jobs
has a default value here (Default value set in fremake.py click options for jobs
- when you don't explicitly define it, 4 prints out). So it might always be "True" or defined.
@singhd789 @ilaflott This is now ready for final review |
merging main in, before doing the reverse
Describe your changes
Added tests in fre/make/null_example for checkout
Issue ticket number and link (if applicable)
creating this so Dana can help with debugging of issues with paths
Checklist before requesting a review
@singhd789