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

"edit on github" / "view on github" links for "stable" -> 404 #1820

Closed
ThomasWaldmann opened this issue Nov 16, 2015 · 49 comments · Fixed by #4052
Closed

"edit on github" / "view on github" links for "stable" -> 404 #1820

ThomasWaldmann opened this issue Nov 16, 2015 · 49 comments · Fixed by #4052
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required

Comments

@ThomasWaldmann
Copy link

See there:

http://borgbackup.readthedocs.org/en/stable/

Link at top right points to:
https://github.com/borgbackup/borg/blob/55fef2457e483701254e5b2a7d4a1a60963971a8/docs/index.rst
(which is 404)

Same for the view/edit links in the bottom left box, they also point to same 404 location.

OTOH, the docs shown for "stable" are (currently) correctly showing 0.28.2 release, which is the latest release.

I tried a "stable" rebuild, did not help.

@anarcat
Copy link

anarcat commented Nov 16, 2015

it would seem more reliable to show a link to a branch there, it seems to me...

@anarcat
Copy link

anarcat commented Nov 16, 2015

55fef2457e483701254e5b2a7d4a1a60963971a8 is the hash of the 0.28.2 tag on github:

$ git show 55fef2457e483701254e5b2a7d4a1a60963971a8 | head
tag 0.28.2
Tagger: Thomas Waldmann <[email protected]>
Date:   Sun Nov 15 22:01:29 2015 +0100

tagged/signed release 0.28.2
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAABCgAGBQJWSPKpAAoJECQ6z6lR944BBt4P/2sRtiN7prKpK7dO8QAPuEiM
Tb4gzaLJRf1ZjSGwFGQ+Ma3YXf3OBMG+ZAZIUtn7q4b3BoetuekCIWcGc9FPxOT5

it seems that GH doesn't render that properly transparently, so it could be a github bug as well...

@ThomasWaldmann
Copy link
Author

Ah, interesting. I only looked in git log output for the hash, but the tags do not show up there.

@ThomasWaldmann
Copy link
Author

could somebody please have a look?

@agjohnson
Copy link
Contributor

This is ticket has not been triaged yet, as we have not had time to focus on this. Feel free to supply test cases or a patch that address the issue you are seeing.

@ThomasWaldmann
Copy link
Author

could this be fixed please?

we get new tickets on our tracker all the time because "edit on github" is broken...

could it be related to that I gpg-sign my release tags in git?

@timabbott
Copy link

I'm seeing the same problem with the files written in markdown in project using a mix of markdown and rST (the "edit on GitHub" links go to e.g. foo.rst when the page was generated from e.g. foo.md):
See e.g. http://zulip.readthedocs.org/en/latest/integration-guide.html

@ThomasWaldmann
Copy link
Author

@timabbott are you signing your release tags with gpg, too?

@anarcat
Copy link

anarcat commented Apr 11, 2016

well, i don't know about others but this is fixed here. the original post here says that the stable release page leads to a 404 page (which is still 404). but the stable page now points to a different page which works fine. also, tagged releases work fine too: they leads to a working link.

i wonder if github's support for signed commits fixed this or something.

anyways, it looks to me like the original issue is fixed. @timabbott's issue seems related not to git tags but to the markdown/rst mix, an issue that should be fixed on RTFD's side, not github.

@ThomasWaldmann
Copy link
Author

@anarcat right, it works now for our stable tag. \o/

thus, I am closing this one - please file different bugs in a different issue.

@timabbott
Copy link

Sounds reasonable, opened #2130 for the markdown issue -- I hadn't realized it was a different bug. Thanks!

@ThomasWaldmann
Copy link
Author

@glasserc
Copy link

glasserc commented May 3, 2016

I've also seen this. http://kinto.readthedocs.io/en/stable/ "edit on github" points to https://github.com/Kinto/kinto/blob/9c331e13643a53ca76208f4efa6c9bb651b165b4/docs/index.rst , which 404s. Like @anarcat said, the hash shown is the hash of the tag, rather than that of the commit. Looking at http://readthedocs.org/api/v1/version/1957996/?format=json, I see the problematic hash listed as the "identifier".

@ThomasWaldmann
Copy link
Author

BTW, how about a setting whether one wants to have "edit on github" links or not?

I'ld just switch it off so we would never have to bother with it again. We have a link to github in our docs, so we don't really need that extra link. ;-)

@magopian
Copy link

@ThomasWaldmann your last command should be in another issue I believe ;)

Regarding the issue at hand, i've investigated a bit, and here are my findings:
When checking for the "identifier" to use, there's a special case when the slug is STABLE:
https://github.com/rtfd/readthedocs.org/blob/b5d06e856ec0eea6f340bc078ab747254b516cd0/readthedocs/builds/models.py#L117

This will return the identifier which is the commit hash. This is what's causing problem, because it's the hash of the tag, not of a commit, and thus GitHub doesn't know how to display it. As you can see from the comment there, the issue is that we don't have the original branch name.

So maybe we could store the branch name instead of the tag hash (which seems useless in our case?) in the identifier field.

This should be done in the update_stable_version I guess (specifically line 716 and 726).

I'll submit a PR with this naive fix, to open up the discussion.

@ericholscher
Copy link
Member

This has been fixed (see http://mockingjay.readthedocs.io/en/stable/ -- it properly links to the hash of the commit on github)

@ThomasWaldmann
Copy link
Author

@ericholscher click on first link in top post, then on "edit on github".

@anarcat
Copy link

anarcat commented Mar 5, 2017

this is not fixed. i still see this problem all the time. it could be a bug with github that doesn't properly recognize certain hashes, but it's still a problem regardless.

@ericholscher
Copy link
Member

ericholscher commented Mar 6, 2017

Where did the commit 55fef2457e483701254e5b2a7d4a1a60963971a8 come from for borg backup? GitHub doesn't seem to recognize it, but I imagine it came from the project at some point. Did you delete and retag a release or something?

Either that, or we have a bug that is pulling tags in from random places.

@anarcat
Copy link

anarcat commented Mar 7, 2017

it's not a commit, it's a tag, and it's the whole bug here - github doesn't display tag hashes correctly when you ask them as commits:

$ git show 55fef2457e483701254e5b2a7d4a1a60963971a8
tag 0.28.2
Tagger: Thomas Waldmann <[email protected]>
Date:   Sun Nov 15 22:01:29 2015 +0100

tagged/signed release 0.28.2
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAABCgAGBQJWSPKpAAoJECQ6z6lR944BBt4P/2sRtiN7prKpK7dO8QAPuEiM
Tb4gzaLJRf1ZjSGwFGQ+Ma3YXf3OBMG+ZAZIUtn7q4b3BoetuekCIWcGc9FPxOT5
gL4ebFkAPXZ24KrU+c+QCHgPf80E2DfqNZYQsv2yaEPfr0W0c40vOP9bdhmOgZF4
z1GLUrE/36PbVkcneIar7fu/YRV5Rc0Ohl+5g4se+EOyhXQBCcAQkGFZefTpe4eq
s9uVkBTx6eD15BSlEebv34Fxc0SPAj2+UIcrIPtDPrUVLv62wfNu5btwS+41FxXs
2m7QJKXWQAmsM24NGLePGsFP5RJLg+E3IF1ytAdYh3ecHk5xsvoiD3c/z0MAO7Cm
3ukiWQVVX6xpLjpM0iEzBrtaxTv0zPqh1r9n9ecIv2PNVxwmcJ27C8IgMqZ0SpLS
w6whl2mimv7C4Ww7vG1qTzjRjBzLqAOOdZ9SIwVrzAjCYcCSS8wDSpJeIArjDroc
mwbeZoIrWWz40XBFNOsoJQ2a2s98SvJoyO/y8wSnPrgSAlc+bKqGlKnmRLDEASml
dDhGJMywJhS0U5H9fDkJcPCR0zlRk24RqUVISN63e0C6zjsWfwoOpZYFmLF4tIT2
OBbd64Ca7+OkIktQgXC5ujBioX3QBUF7Oris6yVJINYP/eC9fGNX0oYNasoLMiu0
h2eYx+ToARnXU8QFcWLU
=cjuM
-----END PGP SIGNATURE-----

commit 3a72fbe418e075ec7af8695a50dee50603d09b0d
Author: Thomas Waldmann <[email protected]>
Date:   Sun Nov 15 20:30:58 2015 +0100

    update CHANGES

diff --git a/docs/changes.rst b/docs/changes.rst
index 9d215e63..f60edd59 100644
--- a/docs/changes.rst
+++ b/docs/changes.rst
@@ -16,8 +16,13 @@ Other changes:
 - do not create docs sources at build time (just have them in the repo),
   completely remove have_cython() hack, do not use the "mock" library at build
   time, #384
-- docs: explain how to regenerate usage and API files (build_api or
-  build_usage) and when to commit usage files directly into git, #384
+- avoid hidden import, make it easier for PyInstaller, easier fix for #218
+- docs:
+
+  - add description of item flags / status output, fixes #402
+  - explain how to regenerate usage and API files (build_api or
+    build_usage) and when to commit usage files directly into git, #384
+  - minor install docs improvements
 
 
 Version 0.28.1

so either you:

  1. patch github to make it DTRT here (hahaha, just kidding, you're SOL there)
  2. ask them nicely to fix this (couldn't resist)
  3. handle those cases by hand and inspect the ref given to extract a valid commit... (looks like this is the only option)

@agjohnson
Copy link
Contributor

Testing a version stable, the Edit link gives me a 404 still. The View link still works for me, but if I try to edit this file on GitHub, the edit button is disabled and tells me that "i must be on a branch" -- so perhaps this is a clue.

Reopening.

@agjohnson agjohnson reopened this May 31, 2018
@ThomasWaldmann
Copy link
Author

ThomasWaldmann commented May 31, 2018

can we just remove (or disable by default) the annoying / buggy features like this or that "outdated library version" note?

the problem is not just that readers encounter these bugs and report them HERE, they also frequently report them at the documented software project, causing work for the maintainers of these. and once you have explained it to one user who found it, the next one will find it and open another issue...

@rawtaz
Copy link

rawtaz commented May 31, 2018

@agjohnson I have the exact same symptoms in e.g. https://restic.readthedocs.io/en/stable/040_backup.html. I have asked the project owner of restic to trigger a rebuild of the docs, but haven't heard from him yet on whether or not he did that.

Would be great if stable just pointed to the last release tag instead, that would have solved it by now.

@stsewd
Copy link
Member

stsewd commented May 31, 2018

That's because commits nor tags can't be edited. So I think this is kind of another issue (hide the Edit button when a version isn't a branch).

@rawtaz
Copy link

rawtaz commented May 31, 2018

@stsewd has a good point, one cannot edit tags either. So at the GitHub end we're left with the branches that exist in the repo. Usually this is master and perhaps some other dev branches.

In order for the edit link to have a purpose, perhaps it simply needs to point to the main branch at all times, regardless of which version you are looking at in RTD?

How to deduct/define the "main" branch is another question though. Maybe the answer to it lies in just looking at what branch the Latest version corresponds to.

EDIT: Then again; What happens on GitHub is a separate concern. Perhaps RTD did what it's supposed to do by linking to the proper file in the proper commit/tag/branch, and that's the end of it, in the sense that it's then up to the user to create a branch from that commit/tag if they want to edit. Just like the "Fork me on GitHub" links on project pages just goes to the original repository, instead of actually starting to fork the original repo into the user's GitHub account.

@rawtaz
Copy link

rawtaz commented Jun 1, 2018

The restic doc project was rebuilt in RTD, but the same state still applies, for obvious reasons. Not much else we can do about it suppose :) Thanks for your hard work!

@techtonik
Copy link
Contributor

I am +1 on reticketing the issue. There were some changes since 2015 and the info needs to be researched again.

@stsewd stsewd added Improvement Minor improvement to code Needed: design decision A core team decision is required and removed Bug A bug labels Sep 21, 2018
@stsewd
Copy link
Member

stsewd commented Sep 21, 2018

Yeah, I don't think this is a bug, is more like an improvement, and it needs a design decision about how to deal with links to no editable versions (tags).

@xeroc
Copy link

xeroc commented Sep 28, 2018

On my deployment, it wouldn't even work on master branch?
See http://how.bitshares.works/en/master/

@stsewd
Copy link
Member

stsewd commented Sep 28, 2018

@xeroc I think you are facing another issue #1917

@techtonik
Copy link
Contributor

@stsewd are you sure looking at the BitShares docs:

© Copyright 2018, BitShares Blockchain Foundation. Revision 3c056912.
Built with Sphinx using a theme provided by Read the Docs.

@stsewd
Copy link
Member

stsewd commented Sep 29, 2018

@techtonik @xeroc that's another bug, please see #4671

@xeroc
Copy link

xeroc commented Oct 16, 2018

thanks for the link.

@stsewd
Copy link
Member

stsewd commented Oct 31, 2018

So, I just discovered that we have this, is_editable thing (sphinx only)

https://github.com/rtfd/readthedocs.org/blob/0f5d979c221da7140138359fc06660ebec6fff9b/readthedocs/doc_builder/backends/sphinx.py#L127-L132

It was added here 4619c82#diff-4ade07b2b17f38e60bfcb27301c99934

The commit msg describes what we are seeing now. But it was never used on the conf.py context.

Anyway, I think we can use that now in the flyout menu with github and friends

https://github.com/rtfd/readthedocs.org/blob/0f5d979c221da7140138359fc06660ebec6fff9b/readthedocs/restapi/templates/restapi/footer.html#L79-L88

@Julian
Copy link

Julian commented Mar 13, 2024

Sorry to necro this, perhaps I should open a new issue, but can I ask whether this was reverted, or otherwise whether everyone else sees this working? I'm finding this issue after essentially seeing the same behavior on https://docs.bowtie.report/ (AKA https://bowtie-json-schema.readthedocs.io) -- it's broken on stable, working on latest, and we indeed use annotated tags (which is what it looks like the stable 404 is trying to point to).

I indeed would be 100% fine with Thomas' suggestion (always have edit links point to latest), but just trying to figure out whether there's a regression or otherwise what I should expect to see at this point.

EDIT: to not re-ping anyone, it seems the actual issue here is pradyunsg/furo#668 as of course obvious in retrospect is that this is theme specific!

@stsewd
Copy link
Member

stsewd commented Mar 13, 2024

@Julian please open a new issue with all the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.