HTML Proofer bug

[Joachim-Lebrun]

Pull Request

#44

What happened?

The HTML proofer is still looking for the files in ../assets/eip-XXX while the files have been all updated to ../assets/erc-XXX during the split. Same happens for files ./erc-XXX that generate errors too as the HTML Proofer says they don't exist.
In the PR #44 i updated the path of all assets files and HTML proofer bot blocks the PR with errors.

Relevant log output

Checking 5415 internal links
Checking internal link hashes in 700 files
Ran on 740 files!


For the Links > Internal check, the following failures were found:

* At ./_site/EIPS/eip-3643.html:251:

  internally linking to ./erc-3643.md, which does not exist

* At ./_site/EIPS/eip-3643.html:268:

  internally linking to ./erc-20.md, which does not exist

* At ./_site/EIPS/eip-3643.html:309:

  internally linking to ./erc-173.md, which does not exist

* At ./_site/EIPS/eip-3643.html:364:

  internally linking to ../assets/erc-3643/ONCHAINID/IERC735.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:405:

  internally linking to ../assets/erc-3643/interfaces/IERC3643.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:473:

  internally linking to ../assets/erc-3643/interfaces/IIdentityRegistry.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:475:

  internally linking to ../assets/erc-3643/ONCHAINID/IClaimIssuer.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:475:

  internally linking to ../assets/erc-3643/ONCHAINID/IIdentity.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:528:

  internally linking to ../assets/erc-3643/interfaces/IIdentityRegistryStorage.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:572:

  internally linking to ../assets/erc-3643/interfaces/ICompliance.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:605:

  internally linking to ../assets/erc-3643/ONCHAINID/IClaimIssuer.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:605:

  internally linking to ../assets/erc-3643/ONCHAINID/IIdentity.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:609:

  internally linking to ../assets/erc-3643/interfaces/ITrustedIssuersRegistry.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:641:

  internally linking to ../assets/erc-3643/ONCHAINID/IIdentity.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:645:

  internally linking to ../assets/erc-3643/interfaces/IClaimTopicsRegistry.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:727:

  internally linking to ../assets/erc-3643/ONCHAINID/IERC735.sol, which does not exist

* At ./_site/EIPS/eip-3643.html:728:

  internally linking to ../assets/erc-3643/ONCHAINID/IIdentity.sol, which does not exist

* At ./_site/EIPS/eip-5564.html:289:

  internally linking to ../assets/erc-5564/scheme_ids.md, which does not exist

* At ./_site/EIPS/eip-5564.html:372:

  internally linking to ./erc-6538, which does not exist

* At ./_site/EIPS/eip-5564.html:384:

  internally linking to ../assets/erc-5564/scheme_ids.md, which does not exist

* At ./_site/EIPS/eip-5564.html:575:

  internally linking to ./erc-4337, which does not exist


HTML-Proofer found 21 failures!
Error: Process completed with exit code 1.

[cfries]

Are you sure that this is a bug of the proofer? I realised that many of the ERC markdown files still reference images and documents in the EIP folder. I could fix many of them with regex. See here: #42

[Joachim-Lebrun]

Are you sure that this is a bug of the proofer? I realised that many of the ERC markdown files still reference images and documents in the EIP folder. I could fix many of them with regex. See here: #42

For me it is clearly a bug, as in this case it happens on a file on which i manually updated the references to match the new terminology.
For example, referencing ERC-20 from an other erc-XXX file with the path ./erc-20.md returns error :

internally linking to ./erc-20.md, which does not exist

while in all logic, the path should not be an issue, as the erc-20.md file is clearly in the same folder as the erc-XXX file that references it

[cfries]

Yes. Appears as if this was introduces just recently? Did not saw this behaviour last week. Sorry for having bothers.
It appears as if the HTML page folder is renamed back to EIP,

* At ./_site/EIPS/eip-3643.html:251:

  internally linking to ./erc-3643.md, which does not exist

where it should reside in ERCS/erc-3643.html.

[cfries]

This renaming of the webpages is a bit disturbing, because it makes it a bit hard to check links in a PR on ones local repository. The markdown files should be locally consistent. ( see the proposal at the end of #42 - use relative links for ERCs and absolute links for EIPs)
@lightclient

[cfries]

If you read here under 7 ii) it appears as if they changed the interpretation of the links back to EIP. So you have to link to eip-20 even if it exists under erc-20 in this repository.
#8

[cfries]

So it appears as if it is intended.

[Divide-By-0]

*Several PRs to this repo have failed due to this issue -- that cannot possibly be intended behavior. Here are some possible solutions:

• Can we define symlinks from the old path to the new path in this repo to avoid duplication, and avoid having to update every single link?
• Alternatively we can write a script to do a mass find/replace update every single ERC to point to the right path.
• We can change the build process to move the intended files to the intended location.

It would be great to solve this issue so that ERCs can continue to be developed, let me know which solution you prefer, and if you'd like my help to make the changes into a PR!

[cfries]

I also do not like the current state. My proposal would be to use - for the time being -

• local links to ./erc-XX.md and
• external links to eip-XX.md

in this repository and do the necessary renaming in the build of the webpage. It is a simple reg. exp. that can do this.

However, it is not true that this is a bug and that every single PR failed due to this issue. If you just link to eip-xx.md instead of erc-xx.md the check will be OK. See the latest commit in this PR: #25 (the HTMLProofer has not failed in PR after the latest commit).

PS: I fixed most of the broken links using the scheme above (local links to ERC, external links to EIP) here: #42 - but I believe it was decided to go a different route.

[Joachim-Lebrun]

I also do not like the current state. My proposal would be to use - for the time being -

• local links to ./erc-XX.md and
• external links to eip-XX.md

in this repository and do the necessary renaming in the build of the webpage. It is a simple reg. exp. that can do this.

However, it is not true that this is a bug and that every single PR failed due to this issue. If you just link to eip-xx.md instead of erc-xx.md the check will be OK. See the latest commit in this PR: #25 (the HTMLProofer has not failed in PR after the latest commit).

PS: I fixed most of the broken links using the scheme above (local links to ERC, external links to EIP) here: #42 - but I believe it was decided to go a different route.

In my opinion it is a bug, even if you can easily solve it by referencing eip-XX.md instead of erc-XX.md because the references are done through relative links, not absolute ones.
If you are referencing ./eip-XX.md from a file erc-XX.md in the ERC repository, it SHOULD throw an error because the relative path is just simply wrong. On the other hand referencing ./erc-XX.md SHOULD pass as the relative path is respected.
It is absolutely not normal that the HTML proofer checks in the EIP repository to find a file referenced through relative path ./erc-XX.md from within a file located in https://github.com/ERC-3643/ERCs/blob/master/ERCS/
In facts, if you do that all your relative links are just broken, even if they pass the HTML proofer bot... Just go and open any md file in the ERCs directory and click on the links referencing other ERCs you will get a 404 error, because github will be looking for it through relative path in the ERCs directory.

As a conclusion, it makes no sense to try and get the bot to return a positive check if it means all relative links are broken when you click on them from the md files in the ERCs directory.

[Joachim-Lebrun]

The goal of the HTML proofer is to ensure no link is broken. The goal is not to get the HTML proofer return true on the checks, otherwise it is even better to remove the HTML proofer check tbh

[cfries]

Not sure if I understand your point. My understanding is: It was decisded that the generation of the HTML page performs a renaming of files (which I do not like, and which is the root of this issue).

The HTMLProofer is not checking the files in this repo. It is checking the generated HTML site. (See the error messages).

The bug is not in the HTMLProofer, the issue is that it was decided to rename erc-XX.md to eip-XX.md while building the side (and doing this without adjusting the links in the files).

[Joachim-Lebrun]

What i mean is that the links, if done like that, are all broken on the github side, if you navigate any file in the ERCs directory, the relative links will throw a 404 when you click on them.
On the website everything works as the links are altered to point to the right content. But i believe that the links should be also accessible from navigating on the github files directly, which is not the case right now

[cfries]

Yes. This is a problem. And I proposed an alternative that does not have this problem.

But they know that. It is stated in 7 ii of #8

Even though ERC-X doesn't correctly render in this repository, we're currently building

And it is not an issue of the HTMLProofer.

[Joachim-Lebrun]

@cfries i understand, and i believe you are right, it is not a bug as such as the HTML proofer works the way it was required to work. The problem comes from the latter, the way the HTML Proofer is set at the moment is not correct. It has the benefit to display links correctly on the website, but at the same time the relative links are all broken on the github side.
Your PR #42 seems the best approach.
I believe it makes sense to keep the issue opened for now as it serves as a reminder that this issue is still pending, but should be closed as soon as #42 is merged

[Pandapip1]

To clear up any confusion: the build system to combine the websites currently just renames the ERC-terminology files so that they use EIP-terminology files. It does NOT rewrite the file contents, however, including any links that use ERC-terminology. For the time being, use eip- as the prefix in links.

[Pandapip1]

This is also a temporary measure. I'll be closing this, since we're all in agreement that it's not a good system.