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

Update Sphinx with C++11 literals fix and add fmt::literals API docs #207

Merged
merged 1 commit into from
Oct 13, 2015

Conversation

dean0x7d
Copy link
Contributor

Sphinx 1.3.1 can't parse user-defined literals. This updates the doc build script to the (currently) latest state on the Sphinx master branch which has the fix.

As for the fmt::literals API docs, the examples do most of the explaining. They mirror the fmt::format and fmt::args examples right above them. The description text just points to the equivalent function. I really couldn't think of anything to write in the description that would be clearer than the example.

vitaut added a commit that referenced this pull request Oct 13, 2015
Update Sphinx with C++11 literals fix and add fmt::literals API docs
@vitaut vitaut merged commit 9575e77 into fmtlib:master Oct 13, 2015
@vitaut
Copy link
Contributor

vitaut commented Oct 13, 2015

Thanks for another great PR. Nice that Sphinx has accepted your implementation of UDL support so quickly. I was going to explain how to use a specific version of Sphinx, but I see you've figured out this already.

@dean0x7d
Copy link
Contributor Author

It's a lucky coincidence that Sphinx's C++ domain is being actively developed right now, so the PR got merged right away.

Heads up: Looks like the Sphinx version on Travis wasn't updated, so the auto-uploaded docs look a bit weird.

@vitaut
Copy link
Contributor

vitaut commented Oct 13, 2015

Yeah, I'm looking into it right now.

@vitaut
Copy link
Contributor

vitaut commented Oct 14, 2015

Looks like pip does something crazy here: it installs version 1.3.1 of sphinx when installing dependencies for version 1.4a0.dev-20151014:

Downloading/unpacking sphinx>=1.3 (from sphinx-rtd-theme>=0.1,<2.0->Sphinx==1.4a0.dev-20151014)
  Downloading Sphinx-1.3.1.tar.gz (3.5Mb): 3.5Mb downloaded

(from https://travis-ci.org/cppformat/cppformat/jobs/85336329)

I'll try upgrading pip on Travis because the version used there is pretty old.

@dean0x7d
Copy link
Contributor Author

From the log you linked:

Installing collected packages: six, Jinja2, Pygments, docutils, snowballstemmer, babel, alabaster, sphinx-rtd-theme, Sphinx, MarkupSafe, pytz, sphinx

It installs both uppercase Sphinx and lowercase sphinx. Well, that pip v1.1 on Travis is a few years out of date.

Sorry about all this trouble. Between the macro stringification and this, It seems like every commit I make shakes out some crazy bug somewhere.

@vitaut
Copy link
Contributor

vitaut commented Oct 14, 2015

It installs both uppercase Sphinx and lowercase sphinx.

Good catch. This could be the problem.

Sorry about all this trouble. Between the macro stringification and this, It seems like every commit I make shakes out some crazy bug somewhere.

No problems, it's good to shake out the obscure bugs every now and then =. The documentation build script did have some issues which I'm fixing in the process. Upgrading pip turned out to be a bit of a pain though.

@dean0x7d
Copy link
Contributor Author

Are you sure that this check_call is actually executed?

if LooseVersion(pip.__version__) < LooseVersion('1.5.4'):
    check_call(['pip', 'install', '--upgrade', 'pip'])

I don't see it in the log.
That pip.__version__ does not correspond to the pip in virtualenv, but to the interpreter currently running the script. Virtualenv installs its own internal pip version (1.1 on Travis). From the log, the system pip should be

Get:2 http://us.archive.ubuntu.com/ubuntu/ precise/universe python-pip all 1.0-1build1 [95.1 kB]

even older 1.0, so technically it should still be lower than 1.5.4 but maybe the pip.__version__ string is wonky?

@vitaut
Copy link
Contributor

vitaut commented Oct 15, 2015

You are right, it isn't. The pip.__version__ at that point is actually 6.0.7. I wonder where it comes from?

@vitaut
Copy link
Contributor

vitaut commented Oct 15, 2015

Anyway, pip is up-to-date now. Now sphinx-build breaks on sphinx-rtd-theme which is not used at all =):

pkg_resources.DistributionNotFound: sphinx-rtd-theme>=0.1,<2.0

@vitaut
Copy link
Contributor

vitaut commented Oct 15, 2015

Reported to sphinx-doc/sphinx#2086.

@vitaut
Copy link
Contributor

vitaut commented Oct 16, 2015

The documentation build has been fixed. Two minor issues though:

  • fmt::literals::operator""_format doesn't have an anchor.
  • fmt::format() link is broken.

@dean0x7d dean0x7d deleted the udl-api branch October 16, 2015 22:52
@dean0x7d
Copy link
Contributor Author

Missing anchors for _format and _a

As far as I can tell, it's an issue in Breathe related to the "Duplicate ID" warnings for overloaded functions. If I comment out the wchar_t overload of _format in format.h and request .. doxygenfunction:: operator""_format without parameters, the anchor shows up.

This issue affects UDLs because they only have the id_v2 in Sphinx, unlike older functions which also have id_v1. I'm not sure why this is, but I've tested it by adding a mock id_v1 to UDLs in the Sphinx source and it restores the anchor. However, I'm pretty sure the error is on Breathe's side since the number of ids shouldn't really matter.

I couldn't figure out exactly what's causing the issue in Breathe, but I'm fairly sure it's related to the function overload resolution. As I said, removing the wchar_t _format overload resolves the issue. I guess you're familiar with Breathe so maybe it's something obvious I'm missing. If not, let me know and I'll submit a proper issue report with example code to Breathe.

Broken fmt::format() link

It's pointing to the wrong functions because Sphinx can't handle overloads. Maybe the simplest solution is to remove the link entirely since fmt::format is actually right above it already. Same goes for _a and fmt::arg.

@vitaut
Copy link
Contributor

vitaut commented Oct 19, 2015

Right, I will look into the issue with UDLs in Breathe when time permits. For the second it's probably make sense to open an issue in Sphinx if they don't have one already. Anyway, I've opened #213 so that this is not forgotten as this PR has been closed.

@dean0x7d
Copy link
Contributor Author

I thought I was close before so I had another look at Breathe and I found the issue. I made a quick fix and it works, but it doesn't follow the factory pattern like the rest of the code so it looks like a bit of a hack. I'm not very good with factories. I'm not sure what would be the proper solution within that framework, so I'll leave it to you for a proper fix. The comments in the commit point to the cause of the issue: it's just that the options passed to create_factory_creator are never actually used.

It fixes the UDL issue and also resolves all those SEVERE: Duplicate ID warnings in the doc build log.

@vitaut
Copy link
Contributor

vitaut commented Oct 20, 2015

I can't say that I fully understand all the factory business in Breathe myself, so I opened a PR. Hopefully @michaeljones can guide us there.

@vitaut
Copy link
Contributor

vitaut commented Oct 20, 2015

And thanks for taking time to debug this and providing the fix, @dean0x7d.

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.

2 participants