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

Add sections explaining exts_list options, and self.cfg in EasyBlocks #617

Merged
merged 18 commits into from
Jun 22, 2020

Conversation

kelseymh
Copy link
Contributor

@kelseymh kelseymh commented Apr 17, 2020

Following the discussion in EasyBlocks #3266, I've added a paragraph of text under "extensions parameters" explaining what each exts_list entry contains, and cross-referencing the "build parameters" section of the file.

I tried to use rst2html (provided with Python/3.6.6-foss-2018b) to check my work, but it apparently does not process :ref: directives (e.g., rst2html Implementing-easyblocks.rst generated a bunch of error messages).

Updated 25 Apr: I am also adding a section to Implementing-easyblocks.rst as well, describing how to use the self.cfg dictionary, per #618 .

@kelseymh
Copy link
Contributor Author

This should not be merged as it is. It appears that not all of the "build parameters" can be used directly in exts_list entries, or else I'm just not doing it properly. Either way, it's going to need more than a bare sentence to explain how.

@boegel
Copy link
Member

boegel commented Apr 18, 2020

@kelseymh The documentation under the version-specific directory is automatically regenerated with every EasyBuild release (using the update-all-docs.sh script in docs/scripts), so your additions will get trashed the way they're done currently...

Perhaps it's better to add a subsection in https://easybuild.readthedocs.io/en/latest/Writing_easyconfig_files.html instead?

For which parameters are you seeing problems?

There indeed are a couple of exceptions, one is sources (you need to use source_tmpl instead, which implies that you can only have a single source per extension currently), but that's one we actually want to fix so sources also works as expected for extensions...

@boegel boegel added this to the 4.x milestone Apr 18, 2020
@kelseymh
Copy link
Contributor Author

@boegel wrote:

The documentation under the version-specific directory is automatically regenerated with every EasyBuild release (using the update-all-docs.sh script in docs/scripts), so your additions will get trashed the way they're done currently...

Oops! Hence the directory name :-/ Okay, I can definitely back that out.

Perhaps it's better to add a subsection in https://easybuild.readthedocs.io/en/latest/Writing_easyconfig_files.html instead?

That works for me.

For which parameters are you seeing problems?

There indeed are a couple of exceptions, one is sources (you need to use source_tmpl instead, which implies that you can only have a single source per extension currently), but that's one we actually want to fix so sources also works as expected for extensions...

I was having trouble getting the filename+git_config substructure to work properly, but that's probably due to the sources/source_tmpl mismatch you just mentioned. Once I get this bit to work, then I'll have more confidence writing up guidance for others.

@kelseymh kelseymh changed the title Add paragraph explaining exts_list options. Add paragraph explaining exts_list options (WIP) Apr 21, 2020
@kelseymh
Copy link
Contributor Author

Added '(WIP)' to title to reflect that the organization of my changes needs to be modified per @boegel .

@kelseymh kelseymh marked this pull request as draft April 21, 2020 17:12
…fernences to sources dictionary and git_config.
Fix my various RST mistakes in the new 'Module extensions' section of Writing_easyconfig_files.  Remove spurious double-colon, improve indentation in `exts_list` example, add specific example for using `git_config` here (coming as part of Framework PR #3294.
@kelseymh kelseymh marked this pull request as ready for review April 23, 2020 06:53
@kelseymh kelseymh changed the title Add paragraph explaining exts_list options (WIP) Add paragraph explaining exts_list options Apr 23, 2020
@kelseymh kelseymh marked this pull request as draft April 25, 2020 05:26
@kelseymh kelseymh marked this pull request as ready for review April 25, 2020 21:31
@kelseymh kelseymh changed the title Add paragraph explaining exts_list options Add sections explaining exts_list options, and self.cfg in EasyBlocks Apr 25, 2020
Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

LGTM apart from the single comment

@kelseymh
Copy link
Contributor Author

kelseymh commented May 8, 2020

@ocaisa One question for you -- should these documentation updates be rolled out before the new feature in Framework PR #3294?

Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

Even if you don't actually uses tags, we should give one here so the example gives best practice.

@ocaisa
Copy link
Member

ocaisa commented May 8, 2020

@kelseymh I haven't been close enough attention but of course you're right, easybuilders/easybuild-framework#3294 should come first.

@boegel boegel modified the milestones: 4.x, next release (4.2.2?) Jun 9, 2020
@kelseymh
Copy link
Contributor Author

kelseymh commented Jun 9, 2020

How do I deal with the "codespell" failure on the "Docs build and installation" test?

https://github.com/easybuilders/easybuild/pull/617/checks?check_run_id=754862987#step:5:1

@ocaisa
Copy link
Member

ocaisa commented Jun 9, 2020

Fix the spelling error at https://github.com/easybuilders/easybuild/pull/617/checks?check_run_id=754862987#step:5:17 and add namd to the ignore words list in easybuild/.github/workflows/check_with_codespell.sh

@ocaisa
Copy link
Member

ocaisa commented Jun 9, 2020

@kelseymh argh sorry, Travis ci needs the ignore word as well

@ocaisa
Copy link
Member

ocaisa commented Jun 9, 2020

@kelseymh kelseymh requested a review from ocaisa June 12, 2020 18:14
@kelseymh
Copy link
Contributor Author

@ocaisa @boegel This PR is still listing "one change requested", but I can't figure out what the change is. Following the "See review" takes me to @ocaisa 's comment about using a tag, rather than a branch or SHA in my example, which I implemented. I'm hoping this could get re-reviewed with the current version so it can roll out to 4.2.2 along with the new code it's documenting.

Copy link
Member

@ocaisa ocaisa 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 the effort you've gone to on this @kelseymh , a couple more suggestions but otherwise good to go

…syConfig with easyconfig in new documentation sections.
Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

Going in, thanks @kelseymh !

@ocaisa ocaisa merged commit 2407c0b into easybuilders:develop Jun 22, 2020
@kelseymh kelseymh deleted the 3266_exts_list_options branch June 22, 2020 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants