QEC-pages / QDistRnd

https://qec-pages.github.io/QDistRnd/
GNU General Public License v2.0
15 stars 2 forks source link

Version 0.9.0 still doesn't work due to `doc/mathjax` symlink #36

Closed fingolfin closed 9 months ago

fingolfin commented 10 months ago

I tried importing it again, see https://github.com/gap-system/PackageDistro/pull/852 , but it failed again. Looking a the log:

tarball contains symlinks: ['qdistrnd-0.9.0/doc/mathjax']

This symlink should indeed not be there. I have seen this before, though: are you by chance using GAP installed via Debian (or a Debian derived system like Ubuntu)? Because they unfortunately patch some things in GAP unilaterally, and one of them is a patch for GAPDoc which makes it produces these symlinks.

But these symlinks are not acceptable for a distribution tarball, as on most user systems the symlink won't work (e.g. on macOS; or a non-Debian-based Linux distros. And Windows doesn't even have symlinks...)

I've been meaning to add code to ReleaseTools to detect this problem and reject it at the start, see this issue but the day has only so many hours sighs. Besides, that would just have told you earlier about the issue, but would not by itself provide a solution. Not that just removing the broken symlink from the tarball is not enough, the generated HTML also references it and must be fixed...

Perhaps you could redo the release process with a self-built version of GAP 4.12.2? That could be done like this (assuming bash) to force it to overwrite the existing tarball (which one normally shouldn't do, but it's OK here as the package has not yet been picked up for distribution)

GAP=/path/top/gap-4.12.2./gap /path/torelease-gap-package --force
LeonidPryadko commented 10 months ago

@fingolfin

I was doing this under windows WSL / Ubuntu, and yes, I used a precompiled version of GAP. I have an older Linux box in the office running Ubuntu, but compiling GAP from scratch may take a very long time (my impression from the last time I did it - mostly because of the prerequisites). Will see what I can do...

What if I just use a clean version of GAPDoc? Also, is there a simple command I can use to detect symlinks in the tarball?

Thanks!

Leonid

On Sun, Jan 14, 2024, 15:18 Max Horn @.***> wrote:

I tried importing it again, see gap-system/PackageDistro#852 https://github.com/gap-system/PackageDistro/pull/852 , but it failed again. Looking a the log https://github.com/gap-system/PackageDistro/actions/runs/7511158103/job/20450417123?pr=852 :

tarball contains symlinks: ['qdistrnd-0.9.0/doc/mathjax']

This symlink should indeed not be there. I have seen this before, though: are you by chance using GAP installed via Debian (or a Debian derived system like Ubuntu)? Because they unfortunately patch some things in GAP unilaterally, and one of them is a patch for GAPDoc https://sources.debian.org/src/gap-gapdoc/1.6.6-1/debian/patches/lib-mathjax/ which makes it produces these symlinks.

But these symlinks are not acceptable for a distribution tarball, as on most user systems the symlink won't work (e.g. on macOS; or a non-Debian-based Linux distros. And Windows doesn't even have symlinks...)

I've been meaning to add code to ReleaseTools to detect this problem and reject it at the start, see this issue https://github.com/gap-system/ReleaseTools/issues/95 but the day has only so many hours sighs. Besides, that would just have told you earlier about the issue, but would not by itself provide a solution. Not that just removing the broken symlink from the tarball is not enough, the generated HTML also references it and must be fixed...

Perhaps you could redo the release process with a self-built version of GAP 4.12.2? That could be done like this (assuming bash) to force it to overwrite the existing tarball (which one normally shouldn't do, but it's OK here as the package has not yet been picked up for distribution)

GAP=/path/top/gap-4.12.2./gap /path/torelease-gap-package --force

— Reply to this email directly, view it on GitHub https://github.com/QEC-pages/QDistRnd/issues/36, or unsubscribe https://github.com/notifications/unsubscribe-auth/APF4LTUVC34HHPELNEIDRRDYORRTHAVCNFSM6AAAAABB2N5WMSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA4DAOJVGQYTAOI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

fingolfin commented 10 months ago

Just using a clean GAPDoc should also work.

LeonidPryadko commented 10 months ago

Dear @fingolfin :

I used GAP under Windows WSL/Ubuntu with a fresh AutoDoc and GAPDoc packages and generated version 0.9.1 -- the tar.gz file does not contain any symlinks now.

release-gap-package script complained a bit: curl: (7) Failed to connect to api.github.com port 443 after 1091 ms: Network is unreachable so I had to use -f option, however in this case there was a message (at the very end of the output):

Updating website
Your configuration specifies to merge with the ref 'refs/heads/gh-pages'
from the remote, but no such ref was fetched.

The release QDistRnd 0.9.1 is present at github, but the website at gitgub.io has not been created (actually, it does not exist)...

Leonid

On Sun, Jan 14, 2024 at 9:29 PM Max Horn @.***> wrote:

Just using a clean GAPDoc should also work.

— Reply to this email directly, view it on GitHub https://github.com/QEC-pages/QDistRnd/issues/36#issuecomment-1891326086, or unsubscribe https://github.com/notifications/unsubscribe-auth/APF4LTWOGY3Z4VEFUES4LPDYOS5DLAVCNFSM6AAAAABB2N5WMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJRGMZDMMBYGY . You are receiving this because you commented.Message ID: @.***>

fingolfin commented 10 months ago

Huh, weird, https://qec-pages.github.io/QDistRnd/ indeed says "Site not fond" -- but it was there a few days ago, because the way the package distribution works is that I point it at https://qec-pages.github.io/QDistRnd/PackageInfo.g and then the rest is automated.

Aha, but looking at the list of branches at your repository, there is no gh-pages branch, which is where the website created using GitHubPagesForGAP is normally placed... This fits with "refs/heads/gh-pages" not being available... It seems something or someone deleted that branch. If you still have your gh-pages directory locally, you may be able to restore it by pushing that branch to GitHub. If not you'll need to create it fresh

LeonidPryadko commented 10 months ago

Yes, thank you, it worked. I think I deleted the gh-pages branch on github in order to run some of the scripts w/o errors. Leonid

On Mon, Jan 15, 2024 at 3:07 PM Max Horn @.***> wrote:

Huh, weird, https://qec-pages.github.io/QDistRnd/ indeed says "Site not fond" -- but it was there a few days ago, because the way the package distribution works is that I point it at https://qec-pages.github.io/QDistRnd/PackageInfo.g and then the rest is automated.

Aha, but looking at the list of branches at your repository, there is no gh-pages branch, which is where the website created using GitHubPagesForGAP is normally placed... This fits with "refs/heads/gh-pages" not being available... It seems something or someone deleted that branch. If you still have your gh-pages directory locally, you may be able to restore it by pushing that branch to GitHub. If not you'll need to create it fresh

— Reply to this email directly, view it on GitHub https://github.com/QEC-pages/QDistRnd/issues/36#issuecomment-1892857553, or unsubscribe https://github.com/notifications/unsubscribe-auth/APF4LTVKXBAPAWP4EXVOU43YOWZE7AVCNFSM6AAAAABB2N5WMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJSHA2TONJVGM . You are receiving this because you commented.Message ID: @.***>

fingolfin commented 10 months ago

OK, we are getting somewhere... good news: the package was finally successfully imported, i.e. it passed general validation etc., see https://github.com/gap-system/PackageDistro/pull/852 !

Bad news: the test suite fails as you can see in this log. The initial error is this, everything else follows:

########> Diff in /home/runner/gap/pkg/qdistrnd-0.9.1/tst/qdistrnd01.tst:25
# Input is:
WriteMTXE("matrices/n5_q3_complex.mtx",3,mat,
        "% The 5-qubit code [[5,1,3]]_3",
        "% Generated from h(x)=1+x^3-x^5-x^6",
        "% Example from the QDistRnd GAP package"   : field:=F);
# Expected output:
File matrices/n5_q3_complex.mtx was created
# But found:
Error, PrintTo: cannot open 'matrices/n5_q3_complex.mtx' for output
########

I did not yet dive in closer, but I am pretty sure the problem is that there is no directory matrices in the active working directory...

Specifically, that test file seems to assume the current working dir while the tests are run is the root directory of your package. But that's not guaranteed, and indeed, not the case when the PackageDistro runs the tests (quite deliberately so).

Later in that file you already write

gap> filedir:=DirectoriesPackageLibrary("QDistRnd","matrices");;
gap> lisX:=ReadMTXE(Filename(filedir,"QX80.mtx"),0);;

and I guess this could be adapted -- however, in general it is a bad idea to try to write into the package directory, as it could be write protected (e.g. because the package was installed system wide by a system administrator).

Instead, I would either put the file into the current working directory, or (better), into a directory obtained via DirectoryTemporary (be careful, that function may return a different directory each time it is called).

The first approach is of course simpler to implement, but has the drawback that it clutters the current working directory with this test file, and of course strictly speaking the working dir may not be readable either (although it is in practice in the package distro). The latter approach is "more correct".

Note that the file is also read after being written, so the subsequent ReadMTXE call must be adjusted to read the same file:

########> Diff in /home/runner/gap/pkg/qdistrnd-0.9.1/tst/qdistrnd01.tst:32
# Input is:
lis:=ReadMTXE("matrices/n5_q3_complex.mtx");;  
# Expected output:
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMe\
thodFound
Error, no 1st choice method found for `ReadAll' on 1 arguments
The 1st argument is 'fail' which might point to an earlier problem

########

Note that I do not know if this is the only issue; on the surface everything else might be a follow-up issue, but I can't be certain.

LeonidPryadko commented 9 months ago

@fingolfin Fixed. The Read/Write functions are now tested together, with the file created in a temporary directory. No write access to current or package directory is needed anymore. The other issues have also been addressed (most of them fixed). I just released a new version (0.9.2) with all of the fixes. Hope it works now...