-
Notifications
You must be signed in to change notification settings - Fork 64
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
Use absolute paths for dependencies #576
Conversation
thanks for doing this, @gold2718 ! Can you edit this PR and re-target it to the "develop" branch? |
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.
one tiny comment
scripts/metadata_table.py
Outdated
@@ -348,6 +348,14 @@ def __init__(self, run_env, table_name_in=None, table_type_in=None, | |||
# end if | |||
self.__start_context = ParseContext(context=self.__pobj) | |||
self.__init_from_file(known_ddts, self.__run_env, skip_ddt_check=skip_ddt_check) | |||
# Set absolute path (if available) for all dependencies |
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 this be something like
# Set absolute path (if available) for all dependencies | |
# Set absolute path (including relative path if available) for all dependencies |
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.
The "if available" part here can probably go away now. I was originally going to put this at the end of the __init__
routine and in some instances, there is no path available (this is independent of relative_path
). Now, the code is under the logic where a ParseContext
is always available so I can grab the path for the file. I will edit.
Attempt to get unit tests to pass on GitHub
(they pass on my desktop)
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.
thanks @gold2718
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.
Looks good to me.
Thanks!
@climbfuji can you take a look at this PR if/when you get a moment? |
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.
Looks ok to me. We'll find out if it works out of the box with prebuild when this comes to main (if the UFS is still using prebuild at that time).
Sorry for the delay. I am on PTO with an 8hr time diff until the end of the month |
Thanks @climbfuji ! @gold2718 you can bring this up to the head of develop and merge it whenever! |
Use absolute paths for dependencies
In order to make the dependencies in the CCPP datatable more usable, store absolute pathnames where possible.
User interface changes?: Yes
This may require a change in any build system that uses the dependencies.
Fixes: #575
Testing:
test removed: None
unit tests: passed
system tests: ?
manual testing: ran Fortran tests