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

Introduce developer tools to replace the util scripts #1133

Merged
merged 6 commits into from
Jun 30, 2022

Conversation

woutdenolf
Copy link
Contributor

@woutdenolf woutdenolf commented Jun 25, 2022

Closes #1144 #1131

Currently: apart from the actual sphinx building we have some scripts in utils which in combination with make files and some custom stuff in the github workflows prepare the files for a sphinx build.

In this PR I propose a python based CLI to do all the non-sphinx stuff

https://github.com/woutdenolf/definitions/blob/dev_tools/dev_tools/README.md

Benefits:

  • the CLI has a help and has a README so you don't have to dig through make files and python code to understand what needs to be executed to build the documentation
  • better / explicit directory management
  • constants are defined only once
  • building is always out-of-source
  • rst files are directly generated by python instead of piping the stdout to a file
  • all NXDL rst files are generated by one command, before it was one command per NXDL file
  • speed-up of the anchor list files (saved only once, see previous point)
  • we no longer need to copy the nxdl files and utils for an out-of-source build
  • uses pytest to minimize test code (unittest has lots of boilerplate)
  • easier to make sure CI and a local repo do the same thing

The python scripts in utils directory become obsolete:

  • build_preparation.py: python3 -m dev_tools manual --prepare (prepare sphinx build)
  • nxdl2rst.py: python3 -m dev_tools manual --prepare (prepare sphinx build)
  • dev_nxdl2rst.py: python3 -m dev_tools nxclass nxfluo --print (for development)
  • test_nxdl.py: pytest dev_tools (for testing)
  • test_nxdl2rst.py: pytest dev_tools (for testing)
  • test_suite.py: pytest dev_tools (for testing)
  • nxdl_desc2rst.py: python3 -m dev_tools manual --prepare (prepare sphinx build)
  • nxdl_summary.py: python3 -m dev_tools manual --prepare (prepare sphinx build)
  • types2rst.py: python3 -m dev_tools manual --prepare (prepare sphinx build)
  • units2rst.py: python3 -m dev_tools manual --prepare (prepare sphinx build)
  • dev_units2rst.py:
  • local_utilities.py:

Not included (could be but not in this PR)

  • create_release_notes.py
  • dev_create_release_notes.py
  • update_copyright_date.py

To show the difference with make local:

make local

python3 -m dev_tools manual --prepare --build-root build2
python3 -m dev_tools impatient --prepare --build-root build2

# Difference in generated rst content
python3 -m dev_tools manual --diff

# Difference in directories
diff -qr build build2

@woutdenolf woutdenolf force-pushed the dev_tools branch 14 times, most recently from b3a7945 to aa9ca3b Compare June 25, 2022 16:20
@woutdenolf woutdenolf added the workflows Continuous integration and deployment label Jun 25, 2022
@woutdenolf woutdenolf force-pushed the dev_tools branch 14 times, most recently from 64a7127 to b7fa250 Compare June 26, 2022 06:37
@woutdenolf
Copy link
Contributor Author

woutdenolf commented Jun 30, 2022

I think main is now good for the release. Note, now NXaberration says its status is "contributed definition, extends [NXobject]".

#1154

Seems like CI is not doing exactly what is done locally. I thought we fixed that.

@PeterC-DLS
Copy link
Contributor

PeterC-DLS commented Jun 30, 2022

Just noticed listing_category on main shouldn't be used in L559 as that makes optional/recommeneded wrong (I think).

Yep, NXaberration's fields have become required

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Jun 30, 2022

Just noticed listing_category on main shouldn't be used in L559 as that makes optional/recommeneded wrong (I think).

Yep, NXaberration's fields have become required

I'll fix it.

Edit: wait I though I fixed that ... was it not merged?

Edit: ah there is a second place where use_application_defaults is defined

Edit: related issue #1155 (I'm working on it)

@PeterC-DLS
Copy link
Contributor

Can check NXcsg too.

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Jun 30, 2022

Rebased on main branch to include the CNAME fix of #1154

@woutdenolf
Copy link
Contributor Author

Currently the difference with the main branch: diff.log

git checkout main
make local
mv build buildmain

git checkout dev_tools
make local

diff -r buildmain/manual/ build/manual/

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Jun 30, 2022

Without the auto-generated directive in the diff: diff.log

diff -r buildmain/manual/ build/manual/ -I 'auto-generated'

Looks good to me.

@prjemian
Copy link
Contributor

Captures my attention:

28c28
<     see: attribute; NXDL attribute
---
>     see:attribute; NXDL attribute

Is this deleted whitespace important to Sphinx?

Copy link
Contributor

@PeterC-DLS PeterC-DLS left a comment

Choose a reason for hiding this comment

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

Done a comparison with main and only differences are timestamps and auto-generated. Good work on the massive speed-up, Wout!

@prjemian
Copy link
Contributor

Most of the differences I see are due to trailing white space. Purely cosmetic.

@PeterC-DLS
Copy link
Contributor

$ diff -ubBr -I 'automatically generated' -I auto-generated -I datetime build-main/manual/build/html/ build/manual/build/html/

@prjemian
Copy link
Contributor

Done a comparison with main and only differences are timestamps and auto-generated. Good work on the massive speed-up, Wout!

By my unscientific measure, about 15x faster!

@prjemian prjemian merged commit 3974c90 into nexusformat:main Jun 30, 2022
@prjemian
Copy link
Contributor

I pushed the big green button.

@woutdenolf
Copy link
Contributor Author

Captures my attention:

28c28
<     see: attribute; NXDL attribute
---
>     see:attribute; NXDL attribute

Is this deleted whitespace important to Sphinx?

Hmmm I wonder. I didn't catch that.

@prjemian
Copy link
Contributor

Locally, built the documentation. It's OK in the index under "attribute", among other items, says see NXDL attribute.

@PeterC-DLS
Copy link
Contributor

@PeterC-DLS
Copy link
Contributor

Good to release then with 208 items closed in the milestone!

@woutdenolf
Copy link
Contributor Author

Thanks for the help with validation. It's a big change but the speed-up and easier maintenance in the future is worth it imo.

@prjemian
Copy link
Contributor

That was a great collaboration, y'all. The end product was much stronger for all our contributions.

Not to rain on anyone's parade but we have one necessary step before making the release:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation workflows Continuous integration and deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NXDL Source link is wrong for contributed definitions
3 participants