gelisam / hawk

Haskell text processor for the command-line
Apache License 2.0
361 stars 20 forks source link

"" interpreted as "()" #103

Closed gelisam closed 10 years ago

gelisam commented 10 years ago

As discussed in #95, hawk "" should be a user error.

gelisam commented 10 years ago

On my machine, issue-103 almost passes (I have already posted the output way up in the thread), and if I remove the Hawk evals "" on input "" equals to "" test, it succeeds. I don't know why your version is looking for Hawk.IO, that file should not exist anymore.

melrief commented 10 years ago

On my machine only master is able to run tests. Both develop and issue-103 have the same resource vanished problem.

melrief commented 10 years ago

Still can't understand why the other user can run the tests...I'm working on it

gelisam commented 10 years ago

Could you try 3515fd2331fe0054175d9fbc8726db28f423b0b7? Issue-95 introduced and fixed a different kind of hClose error, the one which was fixed by moving the runtime out of the src folder.

Failing that, since you have a version which works (master) and a version which doesn't (develop), git-bisect should be able to pinpoint the exact change which is causing the issue.

edit: 3515fd2331fe0054175d9fbc8726db28f423b0b7 is just before issue-95 was merged into develop.

melrief commented 10 years ago

Sadly 3515fd2 has the same results. But the idea of test different commits is good, I'm going back to see if I find a working commit. Next test is 293629b.

melrief commented 10 years ago

Uff I tried all the commits from develop HEAD to when resource vanished disappears and apparently the commit cdfa660 (a better way to detect the sandbox used) is giving me this problem. The commit before c70ebfe doesn't have it but still fails with:

Test suite reference: RUNNING...
Examples: 6  Tried: 6  Errors: 0  Failures: 0
Examples: 13  Tried: 13  Errors: 0  Failures: 0
Examples: 168  Tried: 168  Errors: 0  Failures: 0
### Failure in tests/System/Console/Hawk/PreludeTests.hs:13: expression `testCustomPrelude "default" ["-d:", "-m"] "head" "passwd"'
expected: "root"
 but got: "error: Won't compile:"
          "\tCould not find module `System.Console.Hawk.IO'"
          "Perhaps you meant System.Console.ANSI (from ansi-terminal-0.6.1.1)"
          "Use -v to see a list of the files searched for."
          "*** Exception: ExitFailure 1"
Examples: 169  Tried: 169  Errors: 0  Failures: 1
Test suite reference: FAIL
Test suite logged to: dist/test/haskell-awk-1.0.1-reference.log
0 of 1 test suites (0 of 1 test cases) passed.

Now it's late but tomorrow I will try to understand what that commit changed and why this change is giving that problem.

gelisam commented 10 years ago

apparently the commit cdfa660 (a better way to detect the sandbox used) is giving me this problem

Quite plausible, since that commit changes the directory in which hint will look for the runtime. Here is a history of the strategies I have used in order to find that directory.

The oldest technique is to let hint figure out where to look. Since hint only looks in ~/.cabal, this strategy didn't work with sandboxes.

The next strategy was to look at the path to the hawk executable: if hawk is inside cabal-dev/bin, then I should use unsafeRunInterpreterWithArgs to point hint towards ./cabal-dev/packages-*.conf. Since I was only looking for cabal-dev, it wasn't working with modern cabal sandboxes.

The next strategy was to look for both cabal-dev/bin and .cabal-sandbox/bin, so that cabal sandboxes would also be supported. This was much better, but still depends on the current executable being the hawk binary, which wasn't the case in the "./dist/build/reference/reference" test suite.

The next strategy wasn't as accurate. I had special code to detect whether the current executable was named "reference", and if so I looked at the three possible places: ~/.cabal, cabal-dev, and .cabal-sandbox. If the runtime was installed at more than one place, this strategy could easily use the wrong one.

Finally, in cdfa660 I discovered that the autogenerated Paths_haskell_awk module had a function indicating the folder in which the hawk binary was meant to be installed. This was much better, since I could no longer choose wrong.

gelisam commented 10 years ago

The fact that your tests fail from cdfa660 and onwards indicates that you have more than one version of the runtime installed, probably one in ~/.cabal and one in .cabal-sandbox. I suggest adding a print statement at the end of Sandbox.hs in order to see what extraGhcArgs returns before and after cdfa660.

gelisam commented 10 years ago

The commit before (c70ebfe) doesn't have it but still fails

Since the commit message of cdfa660 is "The old method was not working with the new doctests", I think that's probably normal :) I probably committed something, realized that the tests were now broken, then followed-up with a fix.

edit: nope, the tests for c70ebfe0018b836665ea9d7dc19cf716182f720a pass on my machine.

gelisam commented 10 years ago

Could not find module `System.Console.Hawk.IO

Actually, I can reproduce that error if I first install a recent build, then checkout c70ebfe and try to run cabal test without first removing dist and the sandbox. I know it will take a lot more time, but when you checkout a version with a different runtime, you should remove dist and at least {~/.cabal,.cabal-sandbox}/lib/*/haskell-awk-* in order to make sure the tests are running against the correct version of the runtime.

To avoid this problem, maybe we should bump the build version every time we change the runtime, even if we don't plan to release that version.

gelisam commented 10 years ago

Could not find module `System.Console.Hawk.IO

Okay, forget what I just said. This error occurs on c70ebfe whenever a sandbox is used.

melrief commented 10 years ago

(uff this post is too long and OT...)

The commit ab136d9 removes the test for the empty string because now it's not a valid input and because exprSpec doctest already contains the check with arguments [""]. I think this ends the work around this issue, what do you think?

gelisam commented 10 years ago

Does it also reject " "? On 4 Apr 2014 13:42, "Mario Pastorelli" notifications@github.com wrote:

(uff this post is too long and OT...)

The commit ab136d9 https://github.com/gelisam/hawk/commit/ab136d9removes the test for the empty string because now it's not a valid input and because exprSpec doctest already contains the check with arguments [""]. I think this ends the work around this issue, what do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/gelisam/hawk/issues/103#issuecomment-39591524 .

melrief commented 10 years ago

Yes, the check is all isSpace e so it should. Do you want me to commit another couple of tests to test on single and multiple spaces?

gelisam commented 10 years ago

Nah, good enough :) On 4 Apr 2014 17:39, "Mario Pastorelli" notifications@github.com wrote:

Yes, the check is all isSpace e so it should. Do you want me to commit another couple of tests to test on single and multiple spaces?

— Reply to this email directly or view it on GitHubhttps://github.com/gelisam/hawk/issues/103#issuecomment-39614467 .

melrief commented 10 years ago

Merged with 38b6d8f

melrief commented 10 years ago

@gelisam I've done a mistake in merging the branch resulting in a double merge. Not sure why, I first pulled, then merged and then pushed. But now I can see a merge 532f8c5 that shouldn't be there. I'm trying to fix this right now.

gelisam commented 10 years ago

Might it be related to the renaming of develop to master, which required a forced push? On 16 Apr 2014 07:56, "Mario Pastorelli" notifications@github.com wrote:

@gelisam https://github.com/gelisam I've done a mistake in merging the branch resulting in a double merge. Not sure why, I first pulled, then merged and then pushed. But now I can see a merge 532f8c5https://github.com/gelisam/hawk/commit/532f8c5that shouldn't be there. I'm trying to fix this right now.

— Reply to this email directly or view it on GitHubhttps://github.com/gelisam/hawk/issues/103#issuecomment-40590349 .

melrief commented 10 years ago

I can't stop messing up with git :(. The last merge seems correct but not useful: it is there only because my local branch master was not in sync. I could revert it but I don't know if it is a good idea because git is showing local and remote master branch desynchronized until my merge. What do you think?

melrief commented 10 years ago

Actually the commit 38b6d8f merged my patch in a very old version of master, so even if I don't think I can use revert without problems because in the middle there are all you patches...

gelisam commented 10 years ago

Don't worry about it. This kind of thing is bound to happen when somebody (in this case, me!) uses a forced push!

From the history, I can see what happened. Since we were following the gitflow instructions, our develop and master branches were divergent: for each release and each hotfix, master contained a --no-ff commit which did not exist on develop. This develop branch also gets --no-ff commits which don't exist on the feature branches, one for each branch which gets merged, but since those branches die immediately afterwards, this isn't noticeable.

Whenever you merge in a feature branch, you first pull from develop, and since we never directly commit to develop, it's always a fast-forward merge, that is, no merge commit needs to be created. Then, you create an explicit --no-ff merge commit which merges the feature branch into develop.

Then, I force-pushed develop over master, causing your repo and githup's idea of the history of master to diverge: your master points to what is now called stable, and github's master points to what used to be called develop.

Next, you merged issue-103 into your master, that is, into stable. When you tried to push, you couldn't because the upstream's master was no longer equivalent to stable. So you pulled, naturally, and this time it was not a fast-forward merge, it was commit 532f8c56a5c7cb0bd8d19244ad0c21f3a7ee6ace which merged the master which was pointing at stable with the master which was pointing at develop.

What should have happened instead, is after I did the forced push, I should have told you to run those commands:

$ git checkout master
$ git fetch
$ git reset --hard origin/master

With the reset, your repo would be forced to forget its history of master (the one which was pointing to stable), and to use upstream's history instead (the one which was pointing to develop).

Anyway, it's no big deal. The only difference from the "ideal" is that stable and master no longer diverge, like gitflow recommends. I never really understood the purpose of that suggestion anyway. Let's keep it this way, with no commits on stable which don't exist on master, and when we're ready to release a stable version, we'll just fast-forward stable instead of creating a --no-ff commit.