Improved inheritance

[sifferman]

Fixes #639 and fixes #624

In summary, inheritance is no longer implemented by PyYAML's merge key implementation. Instead, it is overridden by a custom FuseSoC implementation

Specifics

• Added fusesoc/capi2/inheritance.py to centralize logic related to inheritance

• utils.yaml_fread() now calls utils.yaml_read() to improve code reusability

• Possibly fixed small bug in utils.yaml_read() by changing data.read() to data. (There is no test in the CI that checks for this case, so I am uncertain whether this change is good)

• utils.yaml_read() now replaces all merge key operators (<<) with an intermediate value of Inheritance.MERGE_OPERATOR, then turns the YAML into a dictionary as usual, then runs the new Inheritance.elaborate_inheritance() function that matches the specifications in Issues with Merge Keys #639

• Gave utils.merge_dict() the flag concat_list_appends_only that, when set to True, will only concatenate arrays that end in "_append". Usually utils.merge_dict() will concatenate all arrays

[olofk]

This is very clever! I have one concern though. Will this confuse other yaml readers? Just as an example, I would love to have a core file parser in JavaScript to embed in websites, but then I would need to implement the same override for that yaml parsing library, right?

[sifferman]

The replacement of << keys should be fine with other YAML readers. But yes, you would have to implement the yaml_merge_2_fusesoc_merge() and elaborate_inheritance() functions. The only weirdness I would expect between versions would be how references are handled. To get it to work with, PyYAML, I needed to run a bunch of deep_copys (though there is probably a better solution)

Also, I think this PR will actually improve compatibility with other YAML readers considering that the merge operator has been depreciated in YAML 1.2 (ref).

[olofk]

Ouch... you're right. I had no idea. Hmm... knowing this, I wonder if we should do this in two stages.

1. Apply your patch, since that's probably more intuitive
2. Eventually come up with some explicit syntax to make reuse easier and at that point deprecate and warn against the use of << to avoid confusing different yaml readers that might implement this differently

[sifferman]

Sounds good! I can resolve the merge conflicts and squash

On how to add an explicit syntax:

In this patch, users can already use <<__FUSESOC_MERGE_OVERLOAD__<< in place of <<. Therefore, the Inheritance.MERGE_OPERATOR could simply be replaced with a user-friendly operator (maybe $< instead of <<__FUSESOC_MERGE_OVERLOAD__<<).

Though I don't really see a need to do this. Right now, if they use the old one (<<), it will be silently renamed to the new operator. But also, as long as << is replaced before the YAML reader sees it, then there shouldn't be any issues with parsing/support. The only issue I see is that users may be confused why FuseSoC treats << slightly differently than how the YAML 1.1 LRM states

[olofk]

Great. Let's start with this. After I wrote my last comment I remembered that I did a quick PoC on an inherit tag earlier this year after discussions with a customer that wanted a feature like this. Their use-case was that they had a lot of cores and wanted to reduce the amount of boilerplate by inheriting from a common template. Long-term I think we could be looking at something like that, which can cover both that use-case and the functionality that people want from the deprecated merge key feature.

[sifferman]

Alright, I believe it's ready. Thanks for the help, @olofk!

[sifferman]

Actually, I just realized Inheritence.yaml_merge_2_fusesoc_merge() is not as good as it should be yet. Let me improve it a bit

[sifferman]

Alright, NOW it should be good

I added the following tests to make sure that only valid merge key operators are replaced with Inheritance.MERGE_OPERATOR

fusesoc/tests/capi2_cores/parser/inheritance.core

Lines 55 to 59 in 636c166

	merge_test1: {<<: *default}
	merge_test2<<: {tools: {2<<: {}}}
	<<merge_test3: {tools: {<<3: {}}}
	merge_test4: {tools: {t4: {t44: <<4, <<: *default}}, <<: *default}
	merge_test5: {<<__FUSESOC_MERGE_OVERLOAD__<<: *default}

I had to modify the regex in yaml_merge_2_fusesoc_merge()

fusesoc/fusesoc/capi2/inheritance.py

Lines 14 to 23 in 636c166

	def yaml_merge_2_fusesoc_merge(capi):
	"""
	Replace YAML merge key operator (<<) with FuseSoC merge operator
	"""
	yaml_merge_pattern = (
	r"((?:\n|{|\{\s*(?:[^{}]*\{[^{}]*\})*[^{}]*\},)\s*)<<(?=\s*:)"
	)
	while re.search(yaml_merge_pattern, capi):
	capi = re.sub(yaml_merge_pattern, r"\1" + Inheritance.MERGE_OPERATOR, capi)
	return capi

[olofk]

Fantastic! Looks perfect. Thanks for your contributions. Picked and pushed!