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

Updated package.gd to add examples for LoadPackage command + Punctuation tweak to CONTRIBUTING.md #155

Merged
merged 7 commits into from
Apr 8, 2015

Conversation

sr-murthy
Copy link

No description provided.

@sr-murthy
Copy link
Author

Hi, the Travis CI build has passed for the latest change to package.gd to add an example for the LoadPackage command. I've used the Sonata package in the example. I hope it's all OK.

@olexandr-konovalov
Copy link
Member

Thanks. Currently Travis builds do not check that the manual may be built, so passing the test does not ensure this. It is advised to call make manuals in the GAP root directory to check that the manuals may be built. The GAPDoc code looks fine to me, I will make some comments in the source code.

@@ -691,6 +691,31 @@ DeclareGlobalFunction( "LoadPackageDocumentation" );
##
## <Description>
## loads the &GAP; package with name <A>name</A>.
## <P/>
## As an example, the following loads the &GAP; subgroups package
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use loads the &GAP; package <Package>SONATA</Package> which provides methods for the construction and analysis of finite nearrings:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be simpler to make Travis check that the manual can be built?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried make manuals from the GAP repo root but received the following message

make: *** No rule to make target `manuals'.  Stop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried make manuals from the GAP repo root but received the following message

make: *** No rule to make target `manuals'. Stop.

Ah, I was assuming that GAP is built already since you need it to build manuals.
So, the full process from a clean checkout is

./configure
make
make manuals

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course I had forgotten to configure and make GAP. I'm making the manuals now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the 'gapdoc' package cannot be accessed by the GAP build in the repo.

Yes. The easiest way to get it working is perhaps to put a symlink pkg in the GAP root directory pointing to the pkg directory from the latest GAP installation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be simpler to make Travis check that the manual can be built?

There are several aspects here. Perhaps this should be an enhancement proposal,
not a hidden side note in the commit, but for now I'll put it here:

  • what do we want to test?
    a) that 'make manuals' does not fail (a common case when it fails
    is an error in GAPDoc syntax)
    b) that there are no unresolved references in the manual
    c) that manual examples run as expected (with default selection of packages).

We run nightly tests using Jenkins CI where failures of types
(a) and (c) are detected automatically and trigger build failure.
We may discuss which of these checks we want to have in Travis.
Perhaps at least (a) would be important to have, to check pull
requests related to GAP manuals.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK it seems to be fine after creating the symlink to /Applications/GAP/gap4r7/pkg/. Here is the output.

make manuals
( echo 'SaveWorkspace( "doc/wsp.g" );' | ./bin/gap-default64.sh -b -m 100m -o 750m -q -x 80 -r  )
true
( cd doc/ref; echo 'Read( "makedocrel.g" );' | \
      ../.././bin/gap-default64.sh -b -m 100m -o 750m -A -q -x 80 -r  -L ../wsp.g > make_manuals.out )
( cd doc/tut; echo 'Read( "makedocrel.g" );' | \
      ../.././bin/gap-default64.sh -b -m 100m -o 750m -A -q -x 80 -r  -L ../wsp.g > make_manuals.out )
( cd doc/changes; echo 'Read( "makedocrel.g" );' | \
      ../.././bin/gap-default64.sh -b -m 100m -o 750m -A -q -x 80 -r  -L ../wsp.g > make_manuals.out )          
( cd doc/ref; echo 'Read( "makedocrel.g" );' | \
      ../.././bin/gap-default64.sh -b -m 100m -o 750m -A -q -x 80 -r  -L ../wsp.g > make_manuals.out )
( cd doc/tut; echo 'Read( "makedocrel.g" );' | \
      ../.././bin/gap-default64.sh -b -m 100m -o 750m -A -q -x 80 -r  -L ../wsp.g > make_manuals.out )
( cd doc/changes; echo 'Read( "makedocrel.g" );' | \
      ../.././bin/gap-default64.sh -b -m 100m -o 750m -A -q -x 80 -r  -L ../wsp.g > make_manuals.out )           
( rm doc/wsp.g )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK it seems to be fine after creating the symlink to /Applications/GAP/gap4r7/pkg/. Here is the output.

Great! The build log is redirected to make_manuals.out, you may also wish to check those files.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, mdfind make_manuals.out shows three versions existing in doc/ref, doc/tut and doc/changes, looks like one each for the reference manual, tutorial and changes.

I've checked all three, and they look OK. I can post the output if you want (about 50 lines in each).

@sr-murthy sr-murthy changed the title Updated package.gd to add example for LoadPackage command + Punctuation tweak to CONTRIBUTING.md Updated package.gd to add examples for LoadPackage command + Punctuation tweak to CONTRIBUTING.md Apr 6, 2015
@sr-murthy
Copy link
Author

Added the LoadPackage examples for Semigroups and Design to package.gd.

## The package name may be appropriately abbreviated as shown in the following
## two examples with the &GAP; packages <Package>Semigroups</Package>, for
## working with semigroups, monoids and associated structures, and
## <Package>Design</Package>, for working with block designs:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The author is using uppercase and writes DESIGN - please follow this style.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK no problem.

@sr-murthy
Copy link
Author

OK, make manuals seems fine, and the commit for the latest change is now included in the PR. Hope it addresses all the outstanding issues.

@@ -691,6 +691,31 @@ DeclareGlobalFunction( "LoadPackageDocumentation" );
##
## <Description>
## loads the &GAP; package with name <A>name</A>.
## <P/>
## As an example, the following loads the &GAP; package
## <Package>Sonata</Package> (case insensitive) which provides methods for the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct this to <Package>SONATA</Package> to match the style chosen by authors. This will also trigger new Travis build which now includes making GAP manuals.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

## working with semigroups, monoids and associated structures, and
## <Package>DESIGN</Package>, for working with block designs:
## <P/>
## <Log><![CDATA[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid that defeats the purpose. I didn't want long banners, but now it's not even clear which package is loaded. I suggested another option yesterday, may we go for it? What about:

The package name may be appropriately abbreviated. For example, <C>LoadPackage("semi");</C> 
will load the <Package>Semigroups</Package> package, and <C>LoadPackage("d");</C> will load the
<Package>DESIGN</Package> package. If the abbreviation can not be uniquely completed, further 
suggestions will be offered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I will do this later.

@olexandr-konovalov
Copy link
Member

Great, thanks - the new Travis build checked that the manual may be built. I suggest to merge this PR and close issue #8 then.

olexandr-konovalov pushed a commit that referenced this pull request Apr 8, 2015
Updated package.gd to add examples for LoadPackage command.

Also, some minor edits in CONTRIBUTING.md
@olexandr-konovalov olexandr-konovalov merged commit 7d4eb9b into gap-system:master Apr 8, 2015
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