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

Generate codemeta.json #483

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

dennisklein
Copy link
Member

@dennisklein dennisklein commented Sep 11, 2023

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.)

@dennisklein dennisklein force-pushed the generate-codemeta branch 3 times, most recently from 86448d9 to 8bb6061 Compare September 11, 2023 18:29
@dennisklein dennisklein force-pushed the generate-codemeta branch 2 times, most recently from 54cb07f to ed16872 Compare September 11, 2023 18:58
Copy link
Member

@ChristianTackeGSI ChristianTackeGSI left a 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}'
Copy link
Member

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}
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

.github/workflows/codemeta.yaml Show resolved Hide resolved
Comment on lines +47 to +48
publish:
needs: validate
Copy link
Member

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?

Comment on lines +5 to +9
paths:
- codemeta.json
- .github/workflows/codemeta.yaml
tags:
- 'v*'
Copy link
Member

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. :-)

Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

Suggested change
run: cat build/codemeta.json
run: diff codemeta.json build/codemeta.json

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.

2 participants