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

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

Closed
fingolfin opened this issue Jan 14, 2024 · 7 comments
Closed

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

fingolfin opened this issue Jan 14, 2024 · 7 comments

Comments

@fingolfin
Copy link
Contributor

I tried importing it again, see gap-system/PackageDistro#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
Copy link
Member

LeonidPryadko commented Jan 15, 2024 via email

@fingolfin
Copy link
Contributor Author

Just using a clean GAPDoc should also work.

@LeonidPryadko
Copy link
Member

LeonidPryadko commented Jan 15, 2024 via email

@fingolfin
Copy link
Contributor Author

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
Copy link
Member

LeonidPryadko commented Jan 15, 2024 via email

@fingolfin
Copy link
Contributor Author

OK, we are getting somewhere... good news: the package was finally successfully imported, i.e. it passed general validation etc., see gap-system/PackageDistro#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
Copy link
Member

@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...

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

No branches or pull requests

2 participants