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

Adds makefile refinements and nyaml as dependency #1336

Merged
merged 5 commits into from
Jan 17, 2024

Conversation

domna
Copy link
Contributor

@domna domna commented Dec 19, 2023

This adds several things:

  • Proper build of nxdl.xml files even if they are not present. This is now completely based on the available yaml files
  • Removal of nyaml for loop
  • Add a new Makefile nyaml.mk for building nyaml targets to be invoked like this make -f nyaml.mk
  • nyaml as a dependency to provide the nyaml2nxdl command. Here is the pypi page of nyaml.

@sanbrock sanbrock added this to the NXDL 2023.10 milestone Dec 20, 2023
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.

It seems like the content of nyaml.mk is duplicated in Makefile

@domna
Copy link
Contributor Author

domna commented Dec 20, 2023

It seems like the content of nyaml.mk is duplicated in Makefile

It is not, the rule is reversed. One is from nxdl.xml to yaml (in nyaml.mk) the other one is from yaml to nxdl.xml (in Makefile). It is split into two files because otherwise make has a cyclic dependency and cannot be run, using another makefile splits the cycle and allows us to use it in both directions.

Here is a comparison of the two rules for the Makefile (top) and nyaml.mk (bottom):

$(BASE_CLASS_DIR)/%.nxdl.xml : $(BASE_CLASS_DIR)/$(NYAML_SUBDIR)/%.yaml
 	nyaml2nxdl $< --output-file $@
$(BASE_CLASS_DIR)/$(NYAML_SUBDIR)/%.yaml : $(BASE_CLASS_DIR)/%.nxdl.xml
 	nyaml2nxdl $< --output-file $@

This is also why the nyaml.mk file is called for the nyaml command:

nyaml:
 	$(MAKE) -f nyaml.mk

@PeterC-DLS
Copy link
Contributor

Ah, sorry, I should look closer!

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.

Looks fine to me

Copy link
Contributor

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

I don't understand what the two new make targets are supposed to do.

Can you add documentation in the README file on how to use them
(I suppose make nxdl and make nyaml) and what they do?

Also they do not work for me:

(py38) denolf@lindenolf:~/dev/nexus/definitions$ make nxdl
make: Nothing to be done for 'nxdl'.

(py38) denolf@lindenolf:~/dev/nexus/definitions$ make nyaml
make -f nyaml.mk
make[1]: Entering directory '/home/denolf/dev/nexus/definitions'
nyaml2nxdl base_classes/NXsubentry.nxdl.xml --output-file base_classes/nyaml/NXsubentry.yaml
Traceback (most recent call last):
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/bin/nyaml2nxdl", line 8, in <module>
    sys.exit(launch_tool())
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/nyaml/cli.py", line 141, in launch_tool
    converter.print_yml(input_file, yaml_out_file, verbose)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/nyaml/nxdl2nyaml.py", line 184, in print_yml
    self.xmlparse(output_yml, xml_tree, depth, verbose)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/nyaml/nxdl2nyaml.py", line 955, in xmlparse
    with open(output_yml, "a", encoding="utf-8") as file_out:
FileNotFoundError: [Errno 2] No such file or directory: 'base_classes/nyaml/NXsubentry.yaml'
make[1]: *** [nyaml.mk:12: base_classes/nyaml/NXsubentry.yaml] Error 1
make[1]: Leaving directory '/home/denolf/dev/nexus/definitions'
make: *** [Makefile:112: nyaml] Error 2

The new additions to the makefile are too complicated for me.
I don't understand at all what's going on. In addition you have
the second makefile to add to the confusion. Can you move the
logic of make nxdl and make nyaml to the dev_tools main CLI
instead and call that in the nxdl and nyaml targets?

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@domna
Copy link
Contributor Author

domna commented Dec 21, 2023

I don't understand what the two new make targets are supposed to do.

They build nyaml files from nxdl and vice versa.

Can you add documentation in the README file on how to use them (I suppose make nxdl and make nyaml) and what they do?

Yes

Also they do not work for me:

(py38) denolf@lindenolf:~/dev/nexus/definitions$ make nxdl
make: Nothing to be done for 'nxdl'.

(py38) denolf@lindenolf:~/dev/nexus/definitions$ make nyaml
make -f nyaml.mk
make[1]: Entering directory '/home/denolf/dev/nexus/definitions'
nyaml2nxdl base_classes/NXsubentry.nxdl.xml --output-file base_classes/nyaml/NXsubentry.yaml
Traceback (most recent call last):
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/bin/nyaml2nxdl", line 8, in <module>
    sys.exit(launch_tool())
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/nyaml/cli.py", line 141, in launch_tool
    converter.print_yml(input_file, yaml_out_file, verbose)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/nyaml/nxdl2nyaml.py", line 184, in print_yml
    self.xmlparse(output_yml, xml_tree, depth, verbose)
  File "/users/denolf/virtualenvs/nexus/ubuntu_20_04/py38/lib/python3.8/site-packages/nyaml/nxdl2nyaml.py", line 955, in xmlparse
    with open(output_yml, "a", encoding="utf-8") as file_out:
FileNotFoundError: [Errno 2] No such file or directory: 'base_classes/nyaml/NXsubentry.yaml'
make[1]: *** [nyaml.mk:12: base_classes/nyaml/NXsubentry.yaml] Error 1
make[1]: Leaving directory '/home/denolf/dev/nexus/definitions'
make: *** [Makefile:112: nyaml] Error 2

Indeed, there was an error. The nyaml folders to build the files into were not created. This slipped me because I tested this in a repo where these files are always present. I added a fix so that the folders are created. It should work now. Thank you for reporting this.

The new additions to the makefile are too complicated for me. I don't understand at all what's going on. In addition you have the second makefile to add to the confusion. Can you move the logic of make nxdl and make nyaml to the dev_tools main CLI instead and call that in the nxdl and nyaml targets?

I would prefer having them in the makefile as this fits exactly the purpose of make: making files.

I addressed all your issues.

@woutdenolf woutdenolf self-requested a review December 21, 2023 15:19
Copy link
Contributor

@woutdenolf woutdenolf 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 changes. The conversion to and from yaml works now with make.

One last request: could you add nyaml/ to .gitignore?

# Build artifacts
build/
makelog.txt
nyaml/

@domna
Copy link
Contributor Author

domna commented Dec 21, 2023

Thanks for the changes. The conversion to and from yaml works now with make.

One last request: could you add nyaml/ to .gitignore?

# Build artifacts
build/
makelog.txt
nyaml/

Added it

@woutdenolf woutdenolf self-requested a review December 22, 2023 09:56
Copy link
Contributor

@woutdenolf woutdenolf 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 your modifications. LGTM.

@sanbrock sanbrock merged commit f22c8f2 into nexusformat:main Jan 17, 2024
4 checks passed
@domna domna deleted the makefile-updates branch January 17, 2024 16:09
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.

4 participants