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

encoding/xml: restore Go 1.4 name space behavior #11841

Closed
rsc opened this issue Jul 23, 2015 · 16 comments
Closed

encoding/xml: restore Go 1.4 name space behavior #11841

rsc opened this issue Jul 23, 2015 · 16 comments
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Jul 23, 2015

There have been changes to the name space behavior in encoding/xml in this cycle. Those changes affect the output of existing programs, possibly breaking them. (I know of at least one real case.) And yet it's still not clear that the name space story is complete. Is it right to make breaking changes at all? Will we make more breaking changes in Go 1.6 to complete or extend the story? Should we make all the breaking changes at once? Should we arrange that you have to opt in to the breaking changes? Should we declare encoding/xml done, like syscall, and require people who want different behavior to use forks?

All this needs to be more carefully considered, and there is not time left for that in the Go 1.5 cycle.

I propose to rewind the name space behavior back to what we had in Go 1.4 and then look at this all very carefully in the Go 1.6 cycle.

@rogpeppe @nigeltao

@gopherbot
Copy link

CL https://golang.org/cl/12570 mentions this issue.

@jeffallen
Copy link
Contributor

I agree with @rsc. I started hacking on this stuff a bit in an attempt to close issues for Go1.5 and started to feel the same way ("you are in a hall of mirrors, and some of them reflect from a universe where the namespaces are all different"), but didn't have enough data to pull the emergency brake.

@jordan2175
Copy link

I do not fully agree.. XML processing, for anything other than really basic stuff, is somewhat broken, especially when namespaces are concerned. We need to start making improvements and if we wait to the magical point in the future when we can do it all at once, then it will NEVER work as we will never get to that magical point. I think right now people are doing hackery to "make it work". This makes code very brittle overall and is what is probably going to break when these changes are made.

Please reconsider. Lets get some of the changes is now, and have everyone test and find what else still needs to be done. Then lets get a high priority items set for it to be finished in 1.6.

@rogpeppe
Copy link
Contributor

I agree that it's a pity that there are breaking changes, but I think it's
still worth making them now.

Note that the most important changes that I made were to the low level
tokenization, which was fundamentally broken to the point where
you could not round-trip any XML containing namespaces through
Encoder/Decoder without producing broken XML. This is now fixed.

I tried to make as few changes as were necessary to the struct marshaling
and unmarshaling, because a) I didn't want to break things and b) that
wasn't what I was trying to fix at the time and I didn't want to get bogged
down in it.

I don't see that any of the above-mentioned issues that are broken as a result of the
low level tokenization changes, and can only think one issue
where existing valid XML output was broken as a result of this (issue #11431)
and that was still a struct-encoding issue.

So, I agree that encoding/xml is broken but I'd argue that the foundations of
it are now sound, and that we don't have to fix everything now. Also,
we'll get to find more issues when more people actually use it.

On the other hand, I do see that it's a pity to have to make breaking changes
twice (even if, strictly speaking, it's within the remit of the compatibility guarantee
because it's fixing bugs). Another solution might be to move the current
version of encoding/xml to somewhere under golang.org/x, do the reversion
and make more breaking changes (with appropriate fanfare) in Go 1.6,
including a potential rethink of the way that namespaces work with structs,
potentially even renaming the package in std so we can remain backwardly compatible.

For us, it won't make too much difference because we have to use a fork of
encoding/xml anyway as we have to use the Go version in Ubuntu LTS.

@jordan2175
Copy link

Thanks for making those changes @rogpeppe I am grateful for them. I just want to see them put in to 1.5. Please!

@nigeltao
Copy link
Contributor

@rsc's proposed https://go-review.googlesource.com/#/c/12570/ doesn't revert all of the encoding/xml changes since 1.4. It says:


This CL reverts:

5ae822b encoding/xml: minor changes
bb7e665 encoding/xml: fix xmlns= behavior
9f9d66d encoding/xml: fix default namespace of tags
b69ea01 encoding/xml: fix namespaces in a>b tags
3be158d encoding/xml: encoding name spaces correctly

and adjusts tests from

a9dddb5 encoding/xml: add more EncodeToken tests.


These git hashes map to gerrit CLs:

5ae822b https://go-review.googlesource.com/11832
bb7e665 https://go-review.googlesource.com/11635
9f9d66d https://go-review.googlesource.com/6927
b69ea01 https://go-review.googlesource.com/5910
3be158d https://go-review.googlesource.com/2660
a9dddb5 https://go-review.googlesource.com/4167

These are all about encoding in-memory structures to the wire format, right? Decoding from the wire to in-memory is unchanged? Or did you (@rogpeppe) make tokenization fixes for decoding as well as encoding?

I'm inclined to do the conservative thing and roll back, and provide an interim golang.org/x package during the 1.6 cycle if there's demand for it.

CC @rsto for any golang.org/x/net/webdav implications. At the least, we will need to update some golden tests.

@rsto
Copy link
Contributor

rsto commented Jul 24, 2015

In terms of net/webdav it would be a pity to see 3be158d https://go-review.googlesource.com/2660 rolled back, but even if we can't find a workaround in net/webdav most WebDAV clients should manage.

That being said, it would be great to have a temporary fork of encoding/xml at golang.org/x, and I'd volunteer time to support maintenance.

@nigeltao I'll wait until 12570 lands and a decision has been made about any interim fork of encoding/xml.

@ebfe
Copy link
Contributor

ebfe commented Jul 24, 2015

It would be really unfortunate if we had to wait another 6 month for
@rogpeppe s namespace prefix changes.

@jordan2175
Copy link

More like tragic if we have to wait another 6 months.

@davecheney
Copy link
Contributor

I believe this should be a permanent fork, encoding/xml should be
considered "finished" and new development and future improvements should
occur elsewhere.

On Fri, 24 Jul 2015 04:54 Robert Stepanek notifications@github.com wrote:

In terms of net/webdav it would be a pity to see 3be158d
3be158d
https://go-review.googlesource.com/2660 rolled back, but even if we can't
find a workaround in net/webdav most WebDAV clients should manage.

That being said, it would be great to have a temporary fork of
encoding/xml at golang.org/x, and I'd volunteer time to support
maintenance.

@nigeltao https://github.com/nigeltao I'll wait until 12570 lands and a
decision has been made about any interim fork of encoding/xml.


Reply to this email directly or view it on GitHub
#11841 (comment).

@rogpeppe
Copy link
Contributor

If encoding/xml is to be considered "finished" then all the more reason to
go with the fixes I've made, because it is seriously broken without them.

All I have done is fixed bugs. AFAICS all the currently extant issues
were present before I did so.

Fixing more bugs would break more code. I still haven't seen an example
of some code which is broken now where it was working before.

FWIW I think it's well worth having encoding/xml in the standard library.

@jordan2175
Copy link

I am not sure how you could ever consider something so broken to be FINISHED... It should not be marked finished until it actually works and does what the community needs it to do. I think most of us are not yet expecting it to be fully featured like the Java version. But it needs to do most of the normal things. The fact that namespaces do not work is a big problem.

@davecheney
Copy link
Contributor

"finished" was a poor choice of words, hence the quotes.

My position is the limitations of the Go 1 contract, as well as implicit guarantees on previous behaviour that we only discover after the fact, will make it extremely difficult for the XML package to evolve.

I believe a better solution would be to develop a new XML package outside the standard repo.

@jordan2175
Copy link

If we do this, which I can see your rational, then we need to mark the current one as broken, and put a link to the new one and say use that. We do not want people building tooling around an official package that is broken and then having them say, "well Go must not be that stable or not very good because this official core package is broken and no one has fixed it.". Having been in software development for a very long time I would argue that breaking changes in minor release are justified and okay, when the original intent of the package does not work and some how made it through QA.

What I am saying is, if we were to survey the community, and ask them..... "Do you want the main repo fixed, even if it requires breaking changes, or do you want a new parallel package that may never get merged back in to the main repo"? My guess is the vast majority would just say, "Fix the main repo".

@nigeltao
Copy link
Contributor

I thought the question wasn't a choice between "fix the main repo now (with breaking changes)" or "use a separate package forever". I thought it was between "make breaking changes twice, with Go 1.5 and Go 1.6" or "make breaking changes only once, in Go 1.6, and in the meantime, supply a separate, temporary package for those who want it (but wind it down once Go 1.6 is out)".

@rsc
Copy link
Contributor Author

rsc commented Jul 27, 2015

I don't believe we should freeze encoding/xml completely. As time goes on, of course, the bar for changes to any standard packages keeps going up. But there are clearly things that are not right about the handling of name spaces, and it would be good to fix those, provided we can figure out what the right behaviors are. My concern for Go 1.5 is that I don't believe we actually know those answers, and the release is upon us. I do want to try to understand name space behavior better for the next cycle and try to make a fix. I hope that a long term fork only for more sensible name space behavior is not necessary.

It's a bit extreme to call waiting for Go 1.6 "tragic". No other packages in the standard library force the use of encoding/xml (unlike, say, net or net/url), so it is trivial for people who need different behavior to use an internal copy of encoding/xml in the interim. @rogpeppe even noted that for Ubuntu they can't assume Go 1.5 so they have to use a copy anyway. Webdav can do the same (x/net/webdav/internal/xml). If you're willing to depend on the vendor experiment you can use your/code/vendor/encoding/xml and not even have to change your import paths.

I would prefer not to see a golang.org/x/exp/xml. I'd rather people make forks with the changes they need instead of trying to have a fork that's all things to all people.

Again, fixing xml name spaces is on my short list for Go 1.6.

@rsc rsc closed this as completed in c0d6d33 Jul 27, 2015
gopherbot pushed a commit to golang/net that referenced this issue Apr 7, 2016
During the Go 1.5 development cycle, this package used to require the
standard library's encoding/xml package from Go 1.5 or later, but
https://go-review.googlesource.com/#/c/12772/ (which was submitted in
July 2015) made an internal fork of encoding/xml, as some
namespace related changes introduced in the Go 1.5 cycle were rolled
back in response to golang/go#11841

Thus, this "go1.4" runtime check is no longer necessary. In the long
term, this package should use the standard library's version, and the
internal fork deleted, once golang/go#13400 is
resolved. We could re-introduce a similar check at that time, although
it could be done at compile time (via a "go1.7" build tag) instead of at
runtime.

Change-Id: I18258443aa3d9b519e23106aedb189f25c35495d
Reviewed-on: https://go-review.googlesource.com/21634
Reviewed-by: Andrew Gerrand <adg@golang.org>
@golang golang locked and limited conversation to collaborators Aug 5, 2016
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants