semantic icon indicating copy to clipboard operation
semantic copied to clipboard

Finish migrating to `pathtype` library.

Open patrickt opened this issue 6 years ago • 11 comments

The migration to pathtype has been overall a pleasant one—it makes functions much more indicative of their purpose, and it’s caught some bugs (though not serious ones). Right now we’re an uneasy hybrid of pathtype solutions and FilePath; we should move off of the latter completely. This involves overhauling Semantic.CLI to use pathtype; the documentation provides a recipe to make optparse-applicative parsers do the right thing (respecting the path semantics; i.e., if we tell a parser to expect an absolute path, the parser will fail unless said path is actually absolute.)

patrickt avatar Sep 29 '19 18:09 patrickt

Is this still relevant? I'd like to pick this issue up. I'm quite comfortable with Haskell, but I'm quite new to open source. I've read the contribution guidelines, any specific advice on this issue? Thanks.

Temurson avatar Jan 24 '20 08:01 Temurson

@Temurson If you’re comfortable with Haskell, go right ahead once https://github.com/github/semantic/pull/439 lands (which handles all the tricky irritating bits that I had paged into my head). You can start by changing blobPath :: Blob -> FilePath to Blob -> Path.AbsRelFile (you may also want to add blobPathString :: Blob -> String as a convenience function). A bigger task would be to change Data.Project to hold Path.AbsRelDir values for its root and exclude directories.

patrickt avatar Jan 24 '20 22:01 patrickt

@Temurson you should feel free to start before #439 lands, at the risk of conflicts if that branch changes significantly; branching off of @patrickt’s branch should help minimize them, tho.

Thanks very much for your interest!

robrix avatar Jan 27 '20 16:01 robrix

#439 has landed, so anyone can go hog wild here, should they wish to do so. Any use of FilePath is eligible.

patrickt avatar Jan 28 '20 21:01 patrickt

@patrickt I tried installing semantic on my machine, but failed. First, I have problems with running cabal v2-build, it gives errors like "unsatisfied constraints directory not empty" with different packages every time. When I turn off parallelism (v2-build --jobs=1) and it builds successfully. Second, calling cabal v2-test has the same problems with building as in the previous paragraph, but even running it without parallelism I get "1339 out of 1545 tests failed". There is a ton of logs, but a quick look suggests that everything related to parsing fails: all things like %foo%.parseA.txt (maybe something else fails too, there is too much to look at). Any hints on what could be wrong? I can open a new issue with more details if necessary.

Temurson avatar Jan 28 '20 22:01 Temurson

@Temurson

it gives errors like "unsatisfied constraints directory not empty" with different packages every time

No idea what that could be; can you dump the error in a gist? Did you run script/bootstrap?

Re. the v2-test stuff, you need to run script/clone-example-repos, or we don’t check out the OSS repos we use for fixture data.

patrickt avatar Jan 28 '20 22:01 patrickt

@patrickt yes, I ran script/bootstrap.

For some reason I cannot reproduce the errors with v2-build anymore. I tried nuking my local semantic directory and cloning it again, then running the same steps in the "getting started" instructions, but now everything builds perfectly. Perhaps some problematic dependencies have already been installed somewhere and now cabal just uses them? Now I see the actual source files of semantic being built during the v2-build, while it didn't even get to this stage before, crashing while installing dependencies.

Running script/clone-example-repos before running the tests did not help. There are still "1339 out of 1545 tests failed". Here is the example error I'm getting.

parses test/fixtures/typescript/corpus/variable.parseA.txt:
FAIL (0.01s)
        Test output was different from 'test/fixtures/typescript/corpus/variable.parseA.txt'. Output of ["git","diff","test/fixtures/typescript/corpus/variable.parseA.txt","/tmp/variable.parseA.txt7169-1299.actual"]:
        diff --git a/test/fixtures/typescript/corpus/variable.parseA.txt b/tmp/variable.parseA.txt7169-1299.actual
        old mode 100755
        new mode 100644

There is a file with path test/fixtures/typescript/corpus/variable.parseA.txt and it looks fine (there is some output inside), but there is no file with the path /tmp/variable.parseA.txt7169-1299.actual. The directory structure looks like this for me: tmp/typescript-examples/..., and similar directories for Go, Python and Ruby. Is this supposed to be like that?

Temurson avatar Jan 28 '20 23:01 Temurson

@Temurson Hmm, that’s very strange. It looks like the parse-examples test is failing for you, but it doesn’t appear to be failing on master. Are you pulled to the newest version, and have you run a cabal update recently?

patrickt avatar Jan 29 '20 16:01 patrickt

@patrickt I sat down determined to solve it once and for all but failed. It is not the parse-examples test-suite, it is the test test-suite that is failing. I found this question, which seems similar to what I'm getting (see error message above, namely "old mode, new mode" part, all error messages I get have this same text). However, running git config core.filemode false and git --global config core.filemode false did not help. Running git status does not show any unstaged changes. I tried re-cloning the repo and repeating the process, still no luck. The parse-examples test-suite passes by the way. Looks like some git / Linux config problem on my machine. I am not sure this conversation suits this issue anymore, but I really want to resolve this and get to the code. Maybe anyone has any clues on what to do here?

Temurson avatar Jan 30 '20 05:01 Temurson

Hi, I am interested in taking it over. I have a question when reading the code.

For ModulePath defined in source-graph Module.hs, is it a relative Path Path.RelFile, or a absolute path Path.AbsFile, or possibly both relative and absolute path Path.AbsRelFile? ( will assume to use AbsRel if without any good reason)

Work in process in https://github.com/github/semantic/pull/519, there is something requiring refactored and still failing tests with parse-example.

zhujinxuan avatar Mar 20 '20 19:03 zhujinxuan

Hi, @patrickt . #519 is ready for review now.

zhujinxuan avatar Mar 23 '20 00:03 zhujinxuan