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

Parameterize dependencies to allow for easier testing against PRs, branches. #124

Merged
merged 8 commits into from
May 23, 2024

Conversation

WardF
Copy link
Collaborator

@WardF WardF commented May 21, 2024

If this does not fit the style or overall design aesthetic, please feel free to reject it with no hurt feelings. I've added this to make my life easier, in a way that will hopefully help other developers XD.

I've parameterized the stanzas in dependencies.cmake, to allow for easier testing against a branch/fork/PR without changing local files. This will reduce the overhead of keeping track of local changes if testing against a change in a dependency, and will reduce the chances of incorrect dependency versions leaking into the development branch.

I've added a set of new environmental variables, [DEPENDENCY]_GIT_REPOSITORY and [DEPENDENCY_GIT_TAG]. These are currently set to the default values in main. They can be overridden when need be. For example:

$ cmake .. -DTUVX_GIT_REPOSITORY="https://github.com/WardF/tuv-x.git" -DTUVX_GIT_TAG=9e4ad

Instead of making these variables options, and muddying the water, I've left them as environmental variables. I've cut down on a lot of redundant code by adding a function at the top of dependencies.cmake, set_git_default().

Feedback welcome! Hopefully this will prove useful, and it will save time on my end by letting me (and others) more easily test musica against changes in dependencies.

@WardF WardF requested review from mattldawson and K20shores May 21, 2024 16:46
Copy link
Collaborator

@mattldawson mattldawson left a 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!

Should we add some developer instructions for how to point to a specific repo/commit, maybe in the README or documentation? Or we can create a separate issue for this.

@WardF
Copy link
Collaborator Author

WardF commented May 21, 2024

looks good to me!

Should we add some developer instructions for how to point to a specific repo/commit, maybe in the README or documentation? Or we can create a separate issue for this.

I'm happy to write up a how-to/syntax overview, if you point me to where you'd like it. If you confirm the README.md, or point me to a different file, I'll go ahead and amend this PR with documentation :). Thanks!

@mattldawson
Copy link
Collaborator

looks good to me!
Should we add some developer instructions for how to point to a specific repo/commit, maybe in the README or documentation? Or we can create a separate issue for this.

I'm happy to write up a how-to/syntax overview, if you point me to where you'd like it. If you confirm the README.md, or point me to a different file, I'll go ahead and amend this PR with documentation :). Thanks!

I think the README should be fine for now, since the codebase is still in early development. David set up some documentation that we could eventually move some of the README info to, but I think it will only be built with our first release.

@WardF
Copy link
Collaborator Author

WardF commented May 21, 2024

Sounds good, change incoming momentarily and then swapping context to other writing!! :)

@WardF
Copy link
Collaborator Author

WardF commented May 21, 2024

I put something in the README.md; feel free to copy edit, reorganize, and change for style and/or content :). Thanks!

@mattldawson
Copy link
Collaborator

I put something in the README.md; feel free to copy edit, reorganize, and change for style and/or content :). Thanks!

Looks great! Thanks!

@WardF
Copy link
Collaborator Author

WardF commented May 23, 2024

Thanks! I'll merge, since both reviewers have approved it!

@WardF WardF merged commit c1cd9c3 into NCAR:main May 23, 2024
1 of 37 checks passed
@WardF WardF deleted the param_git_tag.wif branch May 23, 2024 17:12
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.

3 participants