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

Semantic #38

Merged
merged 13 commits into from
Aug 9, 2018
Merged

Semantic #38

merged 13 commits into from
Aug 9, 2018

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Aug 4, 2018

Ready to merge.

Juanjo Alvarez added 5 commits July 24, 2018 14:04
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux self-assigned this Aug 4, 2018
@juanjux juanjux requested a review from dennwc August 4, 2018 18:34
Juanjo Alvarez added 2 commits August 4, 2018 20:36
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Have some questions, plus the rename is needed.

build.yml Outdated
- path: '/native/build/libs/native-jar-with-dependencies.jar'
test:
run:
# download and use gradle 4.9 since alpine's version is bugged for tests
Copy link
Member

Choose a reason for hiding this comment

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

Tests are running on top of openjdk:8-slim, according to the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Why are they using that if it's the image specificed for "build" that is at the same level as "test"? Adding an image: 'openjdk:8-jre-alpine' inside test will fix it?

Copy link
Member

Choose a reason for hiding this comment

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

This comment is still not addressed. It tells that "gradle from alpine is broken", but an image that runs tests is not Alpine, it's Debian. Are you sure you still need to download manually grade instead of using a version provided by Debian, or by some PPA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, ok, I misunderstood your comment as saying that I shouldn't run that base image for the tests. I'll take a look at this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated.

build.yml Outdated
- path: src/main/sh/native.sh
dest: native
deps:
- 'apk add gradle'
Copy link
Member

Choose a reason for hiding this comment

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

Is it a dependency for running the driver? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vestigial, fixed on next commit.


var Suite = &fixtures.Suite{
Lang: "bash",
Ext: ".bash",
Copy link
Member

Choose a reason for hiding this comment

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

.sh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer .bash for the fixtures since sh could be understood as POSIX-sh and some tests use bash features.

},
Obj{
uast.KeyType: Var("_type2"),
"startOffset": Var("_startOffset2"),
Copy link
Member

Choose a reason for hiding this comment

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

Why is startOffset visible here? Preprocessor should take care of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Randomly trying things at that point since no matter what I do that annotation doesn't match...

"Node": UASTType(uast.Function{}, Obj{
"Type": UASTType(uast.FunctionType{}, Obj{
//"Arguments": Var("args"),
"Arguments": nil,
Copy link
Member

Choose a reason for hiding this comment

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

IsNil()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is that IsNil() defined? I can't find it in the SDK.

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
build.yml Outdated
artifacts:
- path: '/native/build/libs/native-jar-with-dependencies.jar'
test:
image: 'openjdk:8-jre-alpine'
Copy link
Member

Choose a reason for hiding this comment

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

I won't work. See FROM native as native_test line in the Dockerfile.

I don't understand why are you trying to test against Alpine version if your build environment is Debian Slim.

Copy link
Contributor Author

@juanjux juanjux Aug 7, 2018

Choose a reason for hiding this comment

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

Because some comments above you noted on a line of the "test" section:

"Tests are running on top of openjdk:8-slim, according to the config"

You didnt answered my question as reply of that comment so I understood that I should have added it to the test section.

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux changed the title [Do Not Merge] Semantic Semantic Aug 8, 2018
Juanjo Alvarez added 3 commits August 8, 2018 11:21
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux
Copy link
Contributor Author

juanjux commented Aug 9, 2018

@dennwc PR updated to remove the manual gradle download.

@@ -0,0 +1,30 @@
# Gopkg.toml example
Copy link
Member

Choose a reason for hiding this comment

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

These files are probably unused (in native folder)

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux merged commit a9e5245 into bblfsh:master Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants