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

DOC: Add documentation for community contributions #51

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

tbirdso
Copy link
Collaborator

@tbirdso tbirdso commented Jan 6, 2023

Closes #47

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Thanks for doing this Tom.

Can you please add a self-contained description of the contribution in the commit message, body? It helps a lot instead of just saying "Closes #N".

A few in-line suggestions/comments.

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Show resolved Hide resolved
docs/CONTRIBUTING.md Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
@tbirdso
Copy link
Collaborator Author

tbirdso commented Jan 9, 2023

@jhlegarreta Thanks for the revisions!

Can you please add a self-contained description of the contribution in the commit message, body? It helps a lot instead of just saying "Closes #N".

Typically I add a longer commit message, but this PR is limited in scope and therefore the title "Add documentation for community contributions" fully sums up changes.

@jhlegarreta
Copy link
Member

Typically I add a longer commit message, but this PR is limited in scope and therefore the title "Add documentation for community contributions" fully sums up changes.

IMO minimally C&P'ing the subject in these cases is still helpful/conveys the idea of a complete commit message. But anyways.

Copy link
Member

@dzenanz dzenanz 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 on a glance.

@dzenanz
Copy link
Member

dzenanz commented Jan 9, 2023

@tbirdso if a remote module requires some non-standard build dependency (e.g. presence of Perl, or building of specialized library X), it still cannot use this action? It needs to use the old-style code, like in https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/blob/5fd54c1099eb44d5d5fb29eb349003b4cb651255/.github/workflows/build-test-package.yml#L39-L40? Is this correct? I really wish they allowed inclusion of yaml files, as per actions/starter-workflows#245.

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
Add documentation for community reusable workflow contributions.

Closes InsightSoftwareConsortium#47
@tbirdso
Copy link
Collaborator Author

tbirdso commented Jan 9, 2023

if a remote module requires some non-standard build dependency (e.g. presence of Perl, or building of specialized library X), it still cannot use this action? It needs to use the old-style code, like in https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/blob/5fd54c1099eb44d5d5fb29eb349003b4cb651255/.github/workflows/build-test-package.yml#L39-L40? Is this correct? I really wish they allowed inclusion of yaml files, as per actions/starter-workflows#245.

@dzenanz Unfortunately yes, to the best of my knowledge there is not currently a good path to allowing remote modules to inject additional environment setup steps on a case-by-case basis. Some modules may be able to work around this by fetching and installing dependencies as part of CMake config, such as how ITKElastix pulls and builds Elastix in its CMakeLists.txt. For other modules such as IOOMEZarrNGFF it seems like the only path forward for now is to maintain the workflow fork and manually apply incremental updates to mirror the ITKRemoteModuleBuildTestPackageAction commit history as needed.

The issue you linked and its related anchors issue seem like they would present a path forward if implemented, but it looks like the issue hasn't been a priority for the GitHub team over the past couple of years. Open to revisiting if that or another path are made available.

@jhlegarreta
Copy link
Member

#51 (comment) If a "sufficient" number of modules need a given library L, I guess a boolean flag could be added to the workflow. It is somehow suggested in the text Tom proposes that such a feature could be accepted. Not sure how many would be "sufficient". And if they require library L but versions V1 vs V2, we would hit a difficulty there.

@tbirdso tbirdso merged commit 93fb45d into InsightSoftwareConsortium:main Jan 10, 2023
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.

Add CONTRIBUTING.md documentation
3 participants