-
Notifications
You must be signed in to change notification settings - Fork 34
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
Generate codemeta.json
#483
base: dev
Are you sure you want to change the base?
Conversation
86448d9
to
8bb6061
Compare
8bb6061
to
6bc5000
Compare
54cb07f
to
ed16872
Compare
ed16872
to
ff8079b
Compare
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.
Okay. Some comments.
If you want to fix some of them later and consider this a "I need testing for tags" thing, then that's fine.
manipulator.save() | ||
filename = None | ||
if args.outdir is not None: | ||
filename = f'{args.outdir}/{manipulator.default_filename}' |
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 would prefer a pathlib.Path
based approach for this. Yes, not needed for us. But still nicer?
@@ -72,6 +72,11 @@ endif() | |||
if(BUILD_TIDY_TOOL) | |||
add_subdirectory(fairmq/tidy) | |||
endif() | |||
|
|||
add_custom_target(codemeta | |||
python3 ${CMAKE_SOURCE_DIR}/meta_update.py --outdir ${CMAKE_BINARY_DIR} --set-version ${PROJECT_VERSION} |
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 would expect a find_package(2)(Python … something interpreter)
for this, right?
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.
Ya, was considering it, but there was so much noise with CMake's Python detection in the past that I am afraid it will introduce more maintenance work than hardcoding it - this target really does not need to be super platform independent.
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.
Okay, got that! Please add a comment above it to that fact. Something like "meant for release management, not needed to be as portable".
Have you looked into add_custom_command(OUTPUT … MAIN_DEPENDENCY …)
or the SOURCES
part of add_custom_target
?
publish: | ||
needs: validate |
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.
Okay, now you have the dep. Maybe do the moving of the validate step in this commit instead?
paths: | ||
- codemeta.json | ||
- .github/workflows/codemeta.yaml | ||
tags: | ||
- 'v*' |
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 changes the semantics (AND) compared to the "old" validate workflow. Please comment on this in the commit text body. :-)
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.
thx, ye, will revisit this. thought it was OR logic.
- name: generate codemeta.json | ||
run: cmake --build build --target codemeta | ||
- name: print result | ||
run: cat build/codemeta.json |
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.
Maybe?
run: cat build/codemeta.json | |
run: diff codemeta.json build/codemeta.json |
For #482. Replacing zenodo release pull with github push on release is an exercise for another day.
(Once we merge this, I will make a tag to see if the asset upload works. So far untested.)