augeas-tools fails to parse krb5.conf (solution provided)

Bug #1302638 reported by Jurjen Bokma
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
augeas (Ubuntu)
Confirmed
Medium
Unassigned

Bug Description

In Trusty, augtool doesn't parse the default /etc/krb5.conf
There are multiple issues. The lens cannot handle some keywords, it cannot handle heading digits in realm names, and it cannot handle lowercase realms in the [realms] section. After the following patch, it can, and does handle the default krb5.conf. It still doesn't handle all input that is valid to the library, but I don't mind if you ignore that.

Revision history for this message
Jurjen Bokma (j-bokma-t) wrote :
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "output of diff -u krb5.aug.old /usr/share/augeas/lenses/dist/krb5.aug" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

tags: added: patch
Revision history for this message
Robie Basak (racb) wrote :

Thank you for taking the time to report this bug and helping to make Ubuntu better.

Have you sent this patch upstream? And does it cause any changes to the structure of the tree?

Once you've responded, please change the bug status back to New. Thanks!

Changed in augeas (Ubuntu):
status: New → Incomplete
Revision history for this message
Jurjen Bokma (j-bokma-t) wrote :

I'm the one who should do the thanking: I'm using your distro.

I have not sent the changes upstream.

I'm not very familiar with the jargon of parse tree structure. I'd say it obviously causes changes in the structure of the tree: it adds leaves. But I presume the question is meant: "Are there any trees that are parseable by both the unpatched and the patched lens, but which do not give identical trees?"
I do not know whether to answer yes or no. The 'shapes' (in terms of number of edges from each node) will be identical, but some nodes will have differently named types, although the renames are quite trivial: of consequence only to the innards of the lens, not to its output.
I see that my description of it is already getting longer than the patch itself. I'll happily send the patch upstream if you say that's a better way to go about this.

Changed in augeas (Ubuntu):
status: Incomplete → New
Revision history for this message
Robie Basak (racb) wrote :

Thank you for your reply!

> I'm the one who should do the thanking: I'm using your distro.

I guess it's a two way thing; you're contributing, too! :)

Let me give you an example of why I ask the question. Let's say that in Ubuntu, we take your patch. Upstream later decide to achieve the same thing, but in a different way, so the tree looks different. In the future, will behaviour on Ubuntu then differ from upstream behaviour, such that we won't be able to switch Ubuntu to upstream's behaviour without regressing somebody who is depending on Ubuntu's tree structure for krb5.conf? The risk is that we end up with an indefinite maintenance burden on Ubuntu because we accidentally forked upstream.

For this reason, sending the patch upstream is better for us in general. But if there's some particular reason that we should make a change in Ubuntu ahead of upstream, then it can be considered, just with careful thought to the risk I have tried to explain above. A better way would be to cherry-pick patches from upstream's VCS - preferably after an upstream release. Then we know that upstream intend to support the change in terms of backwards compatibility.

Does that make sense? For this reason I think submitting this upstream would be better in this case, where users will see a behavioural change that they may depend on, and that we don't want to be forced to regress in the future.

tags: added: needs-upstream-report
Changed in augeas (Ubuntu):
importance: Undecided → Medium
Revision history for this message
Robie Basak (racb) wrote :

> I'd say it obviously causes changes in the structure of the tree: it adds leaves. But I presume the question is meant: "Are there any trees that are parseable by both the unpatched and the patched lens, but which do not give identical trees?"

Just to be clear, I think even just adding leaves could cause a future backwards compatibility problem, if upstream decide to add leaves with the same meaning but with different names.

Revision history for this message
Raphaël Pinson (raphink) wrote :

<hat type="augeas_dev">
I'll be happy to merge your patch. Please open a PR against https://github.com/hercules-team/augeas and add unit tests for your bug, and I'll merge it.
</hat>

<hat type="ubuntu_dev">
If you think this should be fixed in trusty, I can also get the patch into the package and see if it will pass into the repository as a bugfix (once it's merged upstream).
</hat>

Revision history for this message
Jurjen Bokma (j-bokma-t) wrote :

Ok, I cloned the GitHub repository, forked, making changes.
As for unit tests, it looks to me like an entire lens is the unit of testing, so I would edit lenses/tests/test_krb5.aug to include some previously unparsed in- and output. Is that what you want me to do?

Also, it's not unlikely that I'll need more changes to this particular lens in the near future (weeks to months). Am I right assuming that
- it's better to send multiple PRs than to accumulate, and
- patches that can pass for a bugfix will get into Trusty even after it is released, whereas ones that seriously change the parse tree won't make it into Trusty even if I were to open the PR tomorrow?

Revision history for this message
Raphaël Pinson (raphink) wrote : Re: [Bug 1302638] Re: augeas-tools fails to parse krb5.conf (solution provided)

On Tue, Apr 8, 2014 at 8:40 PM, Jurjen Bokma <email address hidden> wrote:

> Ok, I cloned the GitHub repository, forked, making changes.
> As for unit tests, it looks to me like an entire lens is the unit of
> testing, so I would edit lenses/tests/test_krb5.aug to include some
> previously unparsed in- and output. Is that what you want me to do?
>
>
It's quite common indeed to use the entire lens as the unit of testing.
Just add a new test case after the last one in
`lenses/tests/test_krb5.aug`. Make sure it fails before you patch, and then
add the patch.

> Also, it's not unlikely that I'll need more changes to this particular
> lens in the near future (weeks to months). Am I right assuming that
> - it's better to send multiple PRs than to accumulate, and
>

Indeed, unless they strongly depend on each other, in which case it might
make it hard to manage if there's a need to modify them before merging.

> - patches that can pass for a bugfix will get into Trusty even after it is
> released, whereas ones that seriously change the parse tree won't make it
> into Trusty even if I were to open the PR tomorrow?
>

Yes, that's right. Patches that are not backwards-compatible are less
likely be accepted as bugfix.

Revision history for this message
Jurjen Bokma (j-bokma-t) wrote :

I opened a PR on Github as Raphaël requested.

One more thing: the Trusty source package depends neither on flex nor on bison. I think it should, but I'm not sure enough to open another bug.

Revision history for this message
Jurjen Bokma (j-bokma-t) wrote :

Err.. s/depends/build-depends/

Chuck Short (zulcss)
Changed in augeas (Ubuntu):
status: New → Confirmed
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.