TINKERPOP-1493 Groovy project doesn't build on Windows #457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removed support for user.dir property as it was being prepended to a fully qualified path and the second drive letter was making the path illegal. Made sure JarFile instances were being closed so that Groovy could delete the directory without encountering file locked errors. Exclude Unix scripts from RAT plugin Tinkergraph integration tests fail but can build when skipping integration tests. Hadoop fails either way. Could be environmental on my end.
Cherry-picked d0469a1 to backport from master and 3.2.3. CTR
These were requiring graphs to support integer and they were not marked as such. While most graphs support int, the Bolt protocol on neo4j does not so it caused some trouble over there. CTR
Strangely this method was deprecated back in 3.1.1, but the annotation was not added (just the javadoc). CTR
|
I verified multiple platforms to ensure the changes didn't have any adverse affects.
Code changes look good to me. VOTE: +1 |
|
I didn't encounter any failures with the TinkerGraph integration tests on Windows. |
|
i'm having a tough time getting this to build with docker. not sure why....keep getting failures that seem unrelated to this PR (dies in spark). |
|
@spmallette are you hitting that with |
|
i didn't try with just |
While this is an improvement, it really sorta fixes a bug that can prevent certain tests from passing. By returning a confirmation of session close, it ensures that certain tests will always pass without having to rely on timers to simulate the situation. This change should be non-breaking to driver providers as to this point they had to have been doing simple fire-and-forget. CTR
|
any update on your Docker build @spmallette? still works ok for me. |
|
Sorry to say I'm still getting errors. You might want to rebase this on the latest from tp31, but i'm not so sure that will help as my problems are still in Spark. Here's a sample of the stacktrace from one of the errors: could just be environmental in some way, but i don't seem to get this on the tp31 branch itself. I just had a clean build of that this morning. @dkuppitz could you give this a shot? Also, recognize that exception i pasted? it seems familiar to me. did we ever diagnose that? |
|
I retested again overnight after rebasing the PR on |
|
I finally got a good build on a different system this morning. Of course, I'm still in the weird position of the head of tp31 working on the failing system and consistent failures on this PR. I wonder what that means. let's see what @dkuppitz says on this when he gets a chance to run it. |
|
I just started a fresh build. Regarding the stacktraces: Wasn't the |
|
I remember that we talked about that, but I thought we'd said this version was ok no? |
|
Yep, this one is good (that's the one I have as well and I've never seen This was the related Docker ticket: moby/moby#1295 (closed, but it seems that people occasionally still run into it). |
|
All good here, VOTE: +1 |
|
I'm still in a position where tp31 is fine on docker but this PR is not. I rely on docker pretty heavily, so until I figure out what's wrong with my environment, I'm hesitant to +1 this though I admit that this change really shouldn't affect spark at all (yet somehow it does). |
|
Hmm, that's very unlikely. Check the output of |
|
Btw. I don't think that this error is related to any of our components. The |
|
i didn't think so. I purposely didn't turn anything on that I know of. |
|
|
I think we can get this merged once #498 goes in as it solves my problems with the build it seems. @pauljackson i will ping back here when that merges and request an additional rebase from you that I can then test. Thanks for your patience on this one. |
|
Looks like #498 has merged. @pauljackson if you could kindly rebase this on |
…upmerge and fix in tp32/master. CTR.
|
Sorry, I’m fluent in git (shame). From reading, I gleaned that rebase is a type of merge from one branch to another. Since I created the pull request on tp31 I don’t understand what it means to rebase it on tp31. I’m happy to follow directions if you could give me more explicit steps. Thanks, From: stephen mallette [mailto:notifications@github.com] Looks like #498#498 has merged. @pauljacksonhttps://github.com/pauljackson if you could kindly rebase this on tp31 for me I can give it a final test. — |
|
well - your But it occurs to me now that you are working from a fork and perhaps you just need to merge |
|
Either one appears to work cleanly. Just update first - |
|
good to know they both work. i sorta figured @pauljackson since @robertdale confirmed that |
Removed support for user.dir property as it was being prepended to a fully qualified path and the second drive letter was making the path illegal. Made sure JarFile instances were being closed so that Groovy could delete the directory without encountering file locked errors. Exclude Unix scripts from RAT plugin Tinkergraph integration tests fail but can build when skipping integration tests. Hadoop fails either way. Could be environmental on my end.
|
Thank you Robert. This was adequate, I think, to give me some things to Google.
I’ve run these commands from my local clone of my fork. It looks like that was all that was necessary. No new pull request. When I look at the status of the old pull request in the main repo it has incorporated 24 other commits since Oct 12 and the status of the request is that the Travis build is pending. Here’s the commands, if someone would be kind to review and confirm. Thanks. –Paul
D:\Data\TEMP\@other\tinkerpop>git fetch --all
Fetching origin
Fetching apache
remote: Counting objects: 8094, done.
remote: Compressing objects: 100% (823/823), done.
remote: Total 8094 (delta 1986), reused 1680 (delta 1678), pack-reused 5064
Receiving objects: 100% (8094/8094), 11.66 MiB | 1.53 MiB/s, done.
Resolving deltas: 100% (3030/3030), completed with 560 local objects.
From https://github.com/apache/tinkerpop
* [new branch] TINKERPOP-1211 -> apache/TINKERPOP-1211
* [new branch] TINKERPOP-1235 -> apache/TINKERPOP-1235
* [new branch] TINKERPOP-1292 -> apache/TINKERPOP-1292
* [new branch] TINKERPOP-1363 -> apache/TINKERPOP-1363
* [new branch] TINKERPOP-1372 -> apache/TINKERPOP-1372
+ 302df7c...69877a2 TINKERPOP-1389 -> apache/TINKERPOP-1389 (forced update)
* [new branch] TINKERPOP-1399 -> apache/TINKERPOP-1399
* [new branch] TINKERPOP-1420 -> apache/TINKERPOP-1420
* [new branch] TINKERPOP-1428 -> apache/TINKERPOP-1428
* [new branch] TINKERPOP-1434 -> apache/TINKERPOP-1434
* [new branch] TINKERPOP-1471 -> apache/TINKERPOP-1471
* [new branch] TINKERPOP-1473 -> apache/TINKERPOP-1473
* [new branch] TINKERPOP-1485 -> apache/TINKERPOP-1485
* [new branch] TINKERPOP-1490 -> apache/TINKERPOP-1490
* [new branch] TINKERPOP-1502 -> apache/TINKERPOP-1502
* [new branch] TINKERPOP-1506 -> apache/TINKERPOP-1506
* [new branch] TINKERPOP-1507 -> apache/TINKERPOP-1507
* [new branch] TINKERPOP-1508 -> apache/TINKERPOP-1508
* [new branch] TINKERPOP-1520 -> apache/TINKERPOP-1520
* [new branch] TINKERPOP-1527 -> apache/TINKERPOP-1527
* [new branch] TINKERPOP-1529 -> apache/TINKERPOP-1529
* [new branch] TINKERPOP-1529-variant2 -> apache/TINKERPOP-1529-variant2
* [new branch] TINKERPOP-1530 -> apache/TINKERPOP-1530
* [new branch] TINKERPOP-1534 -> apache/TINKERPOP-1534
* [new branch] TINKERPOP-1538 -> apache/TINKERPOP-1538
* [new branch] TINKERPOP-1538-master -> apache/TINKERPOP-1538-master
* [new branch] TINKERPOP-1538-tp32 -> apache/TINKERPOP-1538-tp32
* [new branch] TINKERPOP-1541 -> apache/TINKERPOP-1541
* [new branch] TINKERPOP-1542 -> apache/TINKERPOP-1542
* [new branch] TINKERPOP-1543 -> apache/TINKERPOP-1543
* [new branch] TINKERPOP-1545 -> apache/TINKERPOP-1545
* [new branch] TINKERPOP-1545-tp32 -> apache/TINKERPOP-1545-tp32
* [new branch] TINKERPOP-1547 -> apache/TINKERPOP-1547
* [new branch] TINKERPOP-1548 -> apache/TINKERPOP-1548
* [new branch] TINKERPOP-1557 -> apache/TINKERPOP-1557
* [new branch] TINKERPOP-1561 -> apache/TINKERPOP-1561
* [new branch] TINKERPOP-1562 -> apache/TINKERPOP-1562
* [new branch] TINKERPOP-1564 -> apache/TINKERPOP-1564
* [new branch] TINKERPOP-919 -> apache/TINKERPOP-919
* [new branch] TINKERPOP-932 -> apache/TINKERPOP-932
* [new branch] example_python_promise -> apache/example_python_promise
227b85c..16180e1 master -> apache/master
4a4c526..4745fe1 tp31 -> apache/tp31
* [new branch] tp32 -> apache/tp32
* [new tag] 3.1.5 -> 3.1.5
* [new tag] 3.2.3 -> 3.2.3
D:\Data\TEMP\@other\tinkerpop>git checkout tp31
Already on 'tp31'
Your branch is up-to-date with 'origin/tp31'.
D:\Data\TEMP\@other\tinkerpop>git rebase upstream/tp31
fatal: Needed a single revision
invalid upstream upstream/tp31
D:\Data\TEMP\@other\tinkerpop>git status
On branch tp31
Your branch is up-to-date with 'origin/tp31'.
nothing to commit, working tree clean
D:\Data\TEMP\@other\tinkerpop>git remote -v
apache https://github.com/apache/tinkerpop.git (fetch)
apache https://github.com/apache/tinkerpop.git (push)
origin https://github.com/pauljackson/tinkerpop.git (fetch)
origin https://github.com/pauljackson/tinkerpop.git (push)
D:\Data\TEMP\@other\tinkerpop>git rebase apache/tp31
First, rewinding head to replay your work on top of it...
Applying: TINKERPOP-1493 Groovy project doesn't build on Windows
D:\Data\TEMP\@other\tinkerpop>git push origin tp31
To https://github.com/pauljackson/tinkerpop.git
! [rejected] tp31 -> tp31 (non-fast-forward)
error: failed to push some refs to 'https://github.com/pauljackson/tinkerpop.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
D:\Data\TEMP\@other\tinkerpop>git pull
Merge made by the 'recursive' strategy.
D:\Data\TEMP\@other\tinkerpop>git push origin tp31
Counting objects: 371, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (174/174), done.
Writing objects: 100% (371/371), 65.98 KiB | 0 bytes/s, done.
Total 371 (delta 174), reused 306 (delta 126)
remote: Resolving deltas: 100% (174/174), completed with 68 local objects.
To https://github.com/pauljackson/tinkerpop.git
47da753..d49f692 tp31 -> tp31
D:\Data\TEMP\@other\tinkerpop>
From: Robert Dale [mailto:notifications@github.com]
Sent: Wednesday, November 23, 2016 9:57 AM
To: apache/tinkerpop
Cc: Paul A. Jackson; Mention
Subject: Re: [apache/tinkerpop] TINKERPOP-1493 Groovy project doesn't build on Windows (#457)
Either one appears to work cleanly. Just update first - git fetch --all.
Obviously, make sure you're on your branch git checkout tp31.
Then either git rebase upstream/tp31 or git merge upstream/tp31.
Finally, push it back to your repo. git push or git push origin tp31 or however it's named.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#457 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAUcSBxnAokrl0MzVFnP5hDC5pN3O6sCks5rBFQ8gaJpZM4KVbcO>.
…________________________________
|
|
I didn't consider that your remote branch was behind. Duh. What should have been done is a forced push Build passes on Linux. Can't test on windows. VOTE: +0.5 |
|
It didn’t look intuitively correct, but I didn’t know what was normal. What’s the easiest way to make this right, right, right? Just start over and create a new pull request?
Thanks,
-Paul
From: Robert Dale [mailto:notifications@github.com]
Sent: Sunday, November 27, 2016 8:41 AM
To: apache/tinkerpop
Cc: Paul A. Jackson; Mention
Subject: Re: [apache/tinkerpop] TINKERPOP-1493 Groovy project doesn't build on Windows (#457)
I didn't consider that your remote branch was behind. Duh. What should have been done is a forced push git push -f origin tp31 to replace your remote branch with your local branch. Instead git ended up doing a merge and the history here looks a little weird because it pulled in all the changes since your changes. But git does the right thing and it merges cleanly on tp31.
Build passes on Linux. Can't test on windows.
VOTE: +0.5
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#457 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAUcSGfiXS20htPRP2UbTre71Si2zbQQks5rCYhqgaJpZM4KVbcO>.
…________________________________
|
|
@pauljackson It's all good. It's only the history here in the PR that looks weird. Since it's just the one commit, it'll be right in git history when it's actually merged. |
|
Well - this isn't a rebase - it is a merge. See the ugly merge commit here: d49f692 For further explanation: So you had it right up to here but then probably got rattled by the message above and decided to then do the following: which will do a The |
|
I reconfirmed with the latest updates and reaffirm my +1 vote from before.
Although this doesn't completely solve TinkerPop builds on Windows, it passes a lot more than it used to. |
|
All tests pass with VOTE +1 |
|
We have enough votes to commit this. I'll handle it today. Thanks everyone. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removed support for user.dir property as it was being prepended to a
fully qualified path and the second drive letter was making the path
illegal.
Made sure JarFile instances were being closed so that Groovy could
delete the directory without encountering file locked errors.
Exclude Unix scripts from RAT plugin
Tinkergraph integration tests fail but can build when skipping
integration tests. Hadoop fails either way. Could be environmental on my
end.