add yanglint feature #193

GrumpyOldTroll · 2019-09-16T23:22:01Z

Hi,

I wrote a "make yanglint" feature that's been useful for me when working on yang-related drafts. I cleaned it up some, and I'd like to get it integrated into your repo so it'll end up merged in the future, if possible.

It greps markdown for some strings and injects the external yang file contents for YANG-MODULE <module>, or the pyang tree for YANG-TREE <module>, or an example json data file contents for YANG-DATA <module> <file.json> during a regular make.

If you run make yanglint or set a VALIDATE_YANG=1 environment variable, it will run yanglint and pyang --lint to report errors, using those same search strings.

There's an example repo that uses this feature here:
https://github.com/GrumpyOldTroll/ietf-taps-yang

I looked through the issues, and this is related to the request in #186, but I think approaches it a little differently than that request seems to suggest:

This feature only works on markdown, as it stands, and I don't think it breaks anything if your draft doesn't use the feature.

Would you be willing to add this, or to give some feedback about next steps to make it acceptable if it's not there yet?

Thanks and regards,
Jake

PS: I hereby dedicate the code for this to the public domain--if you want something more formal lmk

martinthomson

I think that this needs some more work in order to more cleanly integrate. Is there a smaller slice that could be taken?

martinthomson · 2019-09-25T00:31:27Z

doc/SETUP.md

+
+## yanglint / libyang
+
+[`yanglint`](https://github.com/CESNET/libyang/tree/master/tools/lint) is part of the [`libyang`](https://github.com/CESNET/libyang) package. It's required only if you're validating YANG modules with `make yanglint` or with the `VALIDATE_YANG=1` environment variable during make.


Wrap text to 80 columns.

martinthomson · 2019-09-25T00:32:11Z

doc/YANG.md

+will have enhanced error checking when the VALIDATE_YANG environment
+variable is set to 1 or when running `make yanglint`:
+
+ * `YANG-MODULE <ietf-example.yang>`: inserts the text of the .yang file


The first level of bullets should be flush with the left column.

martinthomson · 2019-09-25T00:33:15Z

doc/YANG.md

+will have enhanced error checking when the VALIDATE_YANG environment
+variable is set to 1 or when running `make yanglint`:
+
+ * `YANG-MODULE <ietf-example.yang>`: inserts the text of the .yang file


It's not clear whether this is a feature of the markdown processor or whether make yanglint does this. If it is make yanglint, then that won't be integrated into the build process properly.

martinthomson · 2019-09-25T00:36:43Z

main.mk

 %.xml: %.md
 @h=$$(head -1 $< | cut -c 1-4 -); set -o pipefail; \
 if [ "$${h:0:1}" = $$'\ufeff' ]; then echo 'warning: BOM in $<' 1>&2; h="$${h:1:3}"; \
 else h="$${h:0:3}"; fi; \
+ if grep -q -E '^\s*YANG-(MODULE|DATA|TREE)' $< ; then \


The different markdown formats have provisions for including files directly and you should use that instead of this ad-hoc enhancement.

In that case, your linting step can grep for the different inclusion methods, or look for modules that are directly defines and extract those into temporary files. Validating the resulting files can then be a separate step.

martinthomson · 2019-09-25T00:42:47Z

yang-check.sh

+ fi
+
+ for mis in ${missing}; do
+ bash -e -x -c "rsync -cvz rsync.iana.org::assignments/yang-parameters/${mis}*.yang modules/"


This script forks a lot of bash subscripts unnecessarily. That will result in bad quoting. For instance, if $mis contains a space, this will fail badly. And the * is passed unquoted to rsync. That's probably OK in this specific case, but it's not a good practice generally. You can invoke rsync directly.

I can use rsync directly, sure. I was doing this mainly so that when rsync errors, the call that caused it will be visible, but the quoting is a very good point and I think it should echo instead, thanks.

martinthomson · 2019-09-25T00:53:40Z

yanglint.mk

@@ -0,0 +1,16 @@
+
+CHECK_TARGETS = $(addsuffix .checkyang,$(drafts_source))
+.PHONY: yanglint $(CHECK_TARGETS)


These $(CHECK_TARGETS) shouldn't be phony. Here's the design I'd recommend:

You index all the potential .yang files that need checking. If you are extracting YANG from the draft source (I would start by not doing that), then you will need to generate temporary file names for those files. Then you create a dependency as follows:

yangfiles = $(wildcard *.yang) ## extend this with generated files yangcheckfiles = $(patsubst %.yang,%.checkyang,$(yangfiles)) .PHONY: yanglint yanglint: $(yangcheckfiles) %.yangcheck: %.yang yangcheck.sh $< | tee $@

Or, if you are checking everything as a single unit:

yangfiles = $(wildcard *.yang) ## extend this with generated files .PHONY: yanglint yanglint: .yangcheck .yangcheck: $(yangfiles) yangcheck.sh $^ | tee $@

Extracting temporary files from draft source is expensive and complicated, but this process should be the basis of your processing. I would start with the simple thing.

There are likely three paths here that you need to do the same thing for: yang modules, plus the examples and maybe the tree diagrams. Each should have its own fork of this process.

martinthomson · 2019-09-25T00:56:46Z

yang-check.sh

+fi
+
+if ! which yanglint 2>&1 > /dev/null ; then
+ echo "error in $0: yanglint required; please install libyang:"


This could download and install yanglint itself if it was missing. That would help in CI.

That seems a bit rude and maybe requires privileges to add something in PATH. I guess maybe install into an environment variable location when it's missing and look for it there?

I wasn't suggesting that you put this in /usr/local or anything like that. But you can download and compile the tool and run that copy. You can see an example of that sort of process in https://github.com/w3c/push-api/blob/gh-pages/Makefile

martinthomson · 2019-09-25T01:04:41Z

yang-fetch-imports.sh

+after=${before}
+last=$((before-1))
+
+echo "checking for module dependencies (downloading into modules/...)"


This should write into a dot directory that is added to .gitignore by default, but if an environment variable is set, then it should choose that directory instead. Though the main modules need to be in source control, these extra dependencies should just be downloaded into a scratch space. Having the environment variable means that you can share a cache between drafts just by setting a value.

martinthomson · 2019-09-25T01:07:21Z

yang-inject.sh

+ had_error=1
+ fi
+done
+for mod in ${all_modules}; do


These files could use better quoting hygiene. Bash arrays can help here. Then you would do "${all_modules[@]}".

martinthomson · 2019-09-25T01:09:19Z

yang-check.sh

+ break
+ fi
+ locals="${locals} ${missing}.yang"
+ missing=$( ( yanglint -f json -D -V -p . -p modules ${mod} 2>&1 || true ) | \


This looks like code repetition. Use a function perhaps.

GrumpyOldTroll · 2019-10-01T14:18:23Z

Thanks for the feedback, this is very helpful.

I missed the existence of {::include fname} for kramdown-rfc2629 and <{{include fname}} for mmark. I'll refactor based on those, address your comments, and resubmit.

Just to make sure I understand: I think the main design suggestion is to do a build operation that covers *.yang, instead of extracting explicit references from the .md. I think that makes sense.

martinthomson · 2019-10-10T00:42:09Z

Just to make sure I understand: I think the main design suggestion is to do a build operation that covers *.yang, instead of extracting explicit references from the .md. I think that makes sense.

👍

This doesn't do anything for checking examples, but maybe you can address that in a separate patch. It seems like having examples inline is of more use than the module definitions themselves, so we might have better justification for extracting those, but I'm sure that we can work something out. Pulling examples from the XML might be the best, but that requires some interesting tweaks to the order of operations, so I think we might want to start with the big bang stuff.

billwuqin · 2024-06-13T13:04:28Z

Is there any reason for me not able to run pyang-lint successfully?
https://github.com/ietf-wg-nmop/draft-ietf-nmop-network-incident-yang/blob/main/.github/workflows/lint.yml

martinthomson · 2024-06-14T04:45:01Z

@billwuqin, that's certainly a viable strategy. This pull request is a bit old, which means that it isn't necessarily how you would do it today.

You might do better with integrating the lint more tightly as I describe in the documentation. For something like pyang-lint, you can get automated installation as described on the same page or in the sourcecode.

That means that you would only need to create a requirements.txt and add a rule approximately like:

yang-files := $(wildcard *.yang)
yang-file-markers := $(patsubst %.yang,.%.yang-lint,$(yang-files))
.PHONY: pyang-lint
pyang-lint: $(pyang-lint-files)
.%.yang-lint: %.yang $(DEPS_FILES)
        pyang-lint $< | tee $@
lint:: pyang-lint

(I haven't debugged the above, but I hope that it is clear enough.)

billwuqin · 2024-06-15T05:12:15Z

@billwuqin, that's certainly a viable strategy. This pull request is a bit old, which means that it isn't necessarily how you would do it today.

You might do better with integrating the lint more tightly as I describe in the documentation. For something like pyang-lint, you can get automated installation as described on the same page or in the sourcecode.

That means that you would only need to create a requirements.txt and add a rule approximately like:
yang-files := $(wildcard *.yang)
yang-file-markers := $(patsubst %.yang,.%.yang-lint,$(yang-files))
.PHONY: pyang-lint
pyang-lint: $(pyang-lint-files)
.%.yang-lint: %.yang $(DEPS_FILES)
        pyang-lint $< | tee $@
lint:: pyang-lint
(I haven't debugged the above, but I hope that it is clear enough.)

Thanks, Martin.

second draft for yanglint feature

213e3ba

martinthomson requested changes Sep 25, 2019

View reviewed changes

fixed idnits warnings in generated tree

2a44c53

Repository owner deleted a comment from Andreykarp251187 Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add yanglint feature #193

add yanglint feature #193

GrumpyOldTroll commented Sep 16, 2019

martinthomson left a comment

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

GrumpyOldTroll Oct 1, 2019

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

GrumpyOldTroll Oct 1, 2019

martinthomson Oct 10, 2019

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

martinthomson Sep 25, 2019

GrumpyOldTroll commented Oct 1, 2019

martinthomson commented Oct 10, 2019

billwuqin commented Jun 13, 2024

martinthomson commented Jun 14, 2024

billwuqin commented Jun 15, 2024


		## yanglint / libyang

		[`yanglint`](https://github.com/CESNET/libyang/tree/master/tools/lint) is part of the [`libyang`](https://github.com/CESNET/libyang) package. It's required only if you're validating YANG modules with `make yanglint` or with the `VALIDATE_YANG=1` environment variable during make.

		@@ -0,0 +1,16 @@

		CHECK_TARGETS = $(addsuffix .checkyang,$(drafts_source))
		.PHONY: yanglint $(CHECK_TARGETS)

add yanglint feature #193

Are you sure you want to change the base?

add yanglint feature #193

Conversation

GrumpyOldTroll commented Sep 16, 2019

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrumpyOldTroll commented Oct 1, 2019

martinthomson commented Oct 10, 2019

billwuqin commented Jun 13, 2024

martinthomson commented Jun 14, 2024

billwuqin commented Jun 15, 2024