Mkr serializer - Githubissues

rhmccullough commented 10 years ago

Re-implementation of mKR serializer using raptor_serialize_turtle.c and raptor_turtle_writer.c context->emit_mkr = 1 for mkr, = 0 for turtle in raptor_serialize_turtle.c Ditto for new emit_mkr argument for _prefix and _base procedures in raptor_turtle_writer.c

rhmccullough commented 10 years ago

As far as I know, all tests pass now. Travis CI build fails on your turtle tests. My local computer build fails because of missing raptor2.h file in build. I assume that's because I have not pulled your raptor2 changes into my repository.

dajobe commented 10 years ago

I made several comments but the Travis CI build of this branch failed in the turtle tests, so you'll need to fix that too so that every existing test passes. https://travis-ci.org/dajobe/raptor/jobs/39962376

rhmccullough commented 10 years ago

I made several comments but the Travis CI build of this branch failed in the turtle tests, so you'll need to > fix that too so that every existing test passes. https://travis-ci.org/dajobe/raptor/jobs/39962376

I've been reading all your comments, but not responding to all of them.

I think your comments are very reasonable, and I will make the suggested changes.

The Travis CI failure is: no @prefix for the "a" in Turtle. Is that something that you changed in Raptor2?

dajobe commented 10 years ago

The travis failure is due to this pull request. The GIT head / master branch passes all the tests. So it must be something in your code change that caused it, presumably the code path for serializing turtle isn't entirely correct.

rhmccullough commented 10 years ago

re: missing raptor2.h. I did not run configure because ./autogen.sh ran configure (without arguments). So if I run configure after autogen.sh, the results are different?

dajobe commented 10 years ago

No the results are the same. Is there a src/raptor.h present or not?

rhmccullough commented 10 years ago

Yes, but src/raptor2.h is there when I get the error. It appears to me that the sequence ./autogen.sh ./configure make does not compile the scripts/build-formats.c program, which is when the error occurs.

rhmccullough commented 10 years ago

The more I look at this, the less sure I am about what's happening. I scrolled back thru my printout. Below where autogen.sh said it was running configure without arguments, autogen.sh said it was running configure with args --enable-maintainer-mode --disable-gtk-doc --enable-silent-rules. So apparently it is remembering args from previous builds, even though I did a make clean. And, apparently, I am supposed to ignore autogen.sh when it tells me to do make.

dajobe commented 10 years ago

Don't run configure after autogen.sh. autogen.sh runs configure for you with the correct arguments. If you watch the output of autogen.sh, you'll see it runs --enable-maintainer-mode. It's that which builds build-formats

dajobe commented 10 years ago

autogen.sh does not remember arguments, but it adds the default ones you need: --enable-maintainer-mode --disable-gtk-doc --enable-silent-rules This is detail you need not worry about. The raptor instructions have always said how to build from GIT using autogen.sh this way. Section 2.1 of http://librdf.org/raptor/INSTALL.html You can see all of this happening on any Travis CI run, which I've pointed to you earlier.

rhmccullough commented 10 years ago

re: raptor2.h error:

I reread http://librdf.org/raptor/INSTALL.html and I compared the next-to-last Travis Cl run to my latest build of Raptor 2 on Windows8.1/Cygwin. This is Raptor2 without mkr-serializer, from raptor-master.zip.

In my build, there is an error from a perl script in docs directory: /bin/sh: -chmod: command not found The attached PDF file is a screen shot of the last page of my build output. If the attachment doesn't get through to GitHub, I can send it to your regular email address.

rhmccullough commented 10 years ago

re: bug that caused failed tests 1) I made the error of inserting "_:" in raptor_turtle_emit_blank() before the call of raptor_turtle_writerbnodeid(). 2) Can all insertions of ":" can be avoided by using raptor_turtle_writer_bnodeid() in all the other places in raptor_serialize_turtle.c where a blank node is emitted?

re: merge conflicts I don't know what the conflicts are. When I cloned dajobe/raptor to a temporary directory on my local computer and did "git status", the only file listed was: modified: tests/ntriples/test.nt When I did "vi test.nt" there were no >>>> & <<<<< markers for conflicts. That file is one of about a dozen files which are listed in "Files to commit" on my local computer repository of branch mkr-serializer of rhmccullough/raptor. I have not done "git add" or "git commit" on any of these files, nor have I changed them. I have edited some of them, but have restored them all to the current versions in dajobe/raptor.

my most recent changes: I fixed failed test bug, and I made cosmetic changes to avoid trivial changes (e.g. empty lines) in dajobe/raptor. I have not finished all the changes that you suggested, because I want to understand the "merge conflicts" and unexplained "Files to commit" before proceeding.

re: missing raptor2.h and perl error My Cygwin perl is v5.14.2

dajobe commented 10 years ago

You should be able to switch to raptor_turtle_writer_bnodeid().

Maybe the conflicts are line endings; DOS <> Unix. git status should show something as a diference.

dajobe commented 10 years ago

GitHub now reports you can't automatically merge this, so I think you'll need to merge with origin/master and then fix any conflicts there.

rhmccullough commented 10 years ago

status report: 1) I deleted all the files that you said to remove. 2) I used Brendan Forster's work flow -- Git resolved the merge conflicts automatically. 3) As before, make aborts on missing -chmod and raptor2.h. I reconfigured with --disable-gtk-doc and make succeeds. 4) The next problem: all "check-eval-turtle" tests fail in tests/raptor-2013. I'd appreciate any hints on the source of this problem.

rhmccullough commented 10 years ago

Travis CI build fails immediately because Git removes execute permission from autogen.sh.

rhmccullough commented 10 years ago

After going back to my local repository, all tests pass. I have not yet replaced strcpy() & strcat().

rhmccullough commented 10 years ago

Now using raptor_stringbuffer. All tests pass. Travis CI build fails because of 644 mode of autogen.sh

dajobe commented 10 years ago

As well as autogen.sh there are several scripts/* directory mode changes. Please undo all mode changes.

dajobe commented 10 years ago

tests/ntriples/all-escape.nt also has some kind of change you need to undo

dajobe commented 10 years ago

I ran 'make distcheck' with this branch and it also fails. This is a standard check that the package builds when the build directory is different from the source directory.

In tests/mkr/Makefile.am it seems that the test write to '$$name.out' but the compare uses '$(srcdir)/$$name.out' . These should be the same. Also $$name.mkr should be $(srcdir)/$$name.mkr

After I made those changes I got more failures because rdfq-results.ttl assumes a specific base URI which won't be the case for me or anyone else:

Checking rdfq-results.ttl FAILED
../../utils/rapper -q -i turtle -o mkr ../../../tests/mkr/rdfq-results.ttl - > rdfq-results.out 2> rdfq-results.err
--- rdfq-results.out    2014-11-10 11:50:43.528446529 -0800
+++ ../../../tests/mkr/rdfq-results.mkr 2014-11-10 11:40:34.000000000 -0800
@@ -1,10 +1,10 @@
 @prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> ;
 @prefix rs: <http://jena.hpl.hp.com/2003/03/result-set#> ;

-<file:///home/dajobe/dev/redland/raptor/raptor2-2.0.16/tests/mkr/rdfq-results.ttl> has
+<file:///cygdrive/c/Users/rhm/github/raptor/tests/mkr/rdfq-results.ttl> has
 rs:resultVariable = ["x", "y"],
 rs:size = [4] ;
-result is <file:///home/dajobe/dev/redland/raptor/raptor2-2.0.16/tests/mkr/rdfq-results.ttl> ;
+result is <file:///cygdrive/c/Users/rhm/github/raptor/tests/mkr/rdfq-results.ttl> ;
 result is relation with
 format = [value:1, value:2],
 meaning = {x := $1; y := $2;} ;

You'll need to pass one into the test; it's an argument to rapper. There are other test examples that use it - see BASE_URI definition and use in tests/turtle/Makefile.am

dajobe commented 10 years ago

I should say that we're close to getting it ready and I appreciate all the work you've done and learning you've had to do.

rhmccullough commented 10 years ago

I don't know how to do that. I think all these false changes started with the dos<->unix problem. But I have been unable to change the mode. All of my current files have the execute bits set. But Git keeps changing my 755 mode to 644 mode.

rhmccullough commented 10 years ago

I will have another look at that one.

I noticed that one of your dos-test files has a CR return at the end of the line, and Git removed. I had to put two CRs at the end of the line, and then Git only removed one CR. It still lists that file as a File to be committed every time -- I uncheck it so Git won't remove the CR again.

rhmccullough commented 10 years ago

I never ran 'make distcheck'. I deleted everything from tests/mkr/Makefile.am except the exact serialization test. I guess I have to put some things back in the Makefile.

I presume anything related to docs would also fail, since a build with --enable-gtk-doc always fails on Windows/Cygwin.

rhmccullough commented 10 years ago

1) baseuri problem fixed. 2) I asked GitHub gurus for help on execute bits. 3) When I run 'make distcheck' it fails because gtk-doc fails.

rhmccullough commented 10 years ago

4) tests/ntriples/all-escape.nt is another dos<->unix problem. I tried the same trick of adding 2 CRs. Let me know if it worked this time (possibly sensitive to odd/even byte count).

dajobe commented 10 years ago

With this code plus some commits I made to revert the tests/ntriples/all-escape.nt and the script modes, this branch / Pull Request now passes 'make distcheck'. So we're ready to go, apart from some documentation updates.

I propose we could make this a new pull request with just 1 commit rather than all the to & Fro of the 70 commits here.