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

Improved pathing changes made for Windows Build Support #1739

Merged

Conversation

L0Lmaker
Copy link
Contributor

Pathing changes introduced to support the windows build of gremlin-javascript, gremlin-dotnet, and gremlint would make it so that building Tinkerpop or its modules from anywhere other than the Tinkerpop root would not work.

The changes in this PR would make it so that maven commands such as install, verify, compile and so on would work as expected from outside the root.

@L0Lmaker L0Lmaker marked this pull request as ready for review June 27, 2022 23:26
@lyndonbauto
Copy link
Contributor

LGTM
VOTE+1

Copy link
Member

@FlorianHockmann FlorianHockmann left a comment

Choose a reason for hiding this comment

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

I think this is definitely already a good improvement and most of the tests seem to pass this way, but I still run into problems with the Gherkin tests in gremlin-dotnet when executed from that directory:

mvn verify -pl :gremlin-dotnet,:gremlin-dotnet-tests -P gremlin-dotnet -DincludeNeo4j

leads to errors such as:

java.lang.IllegalStateException: data/tinkerpop-modern.xml does not exist

so it looks like more paths need to be changed to get full support for building and testing from other paths than the project's root.

But again, this is already a good improvement, so we can also merge this from my side without also fully supporting the Gherkin tests already.
Could you however please squash your commits? Would be better to have only one commit here instead of six.

@L0Lmaker
Copy link
Contributor Author

L0Lmaker commented Jun 29, 2022

I think this is definitely already a good improvement and most of the tests seem to pass this way, but I still run into problems with the Gherkin tests in gremlin-dotnet when executed from that directory:

mvn verify -pl :gremlin-dotnet,:gremlin-dotnet-tests -P gremlin-dotnet -DincludeNeo4j

leads to errors such as:

java.lang.IllegalStateException: data/tinkerpop-modern.xml does not exist

so it looks like more paths need to be changed to get full support for building and testing from other paths than the project's root.

I am able to reproduce the same error. I checked with an older version of the code without the windows build support changes in commit 38a3bd7 on 3.5 and the same error exists there as well.

Shall I document this in TINKERPOP-2760, which I made for pending windows tasks or create a new ticket for it?

@L0Lmaker L0Lmaker force-pushed the rithin/an-1190-pathing-errs branch from 00517d3 to 6aa0d21 Compare June 29, 2022 19:44
@L0Lmaker
Copy link
Contributor Author

Could you however please squash your commits? Would be better to have only one commit here instead of six.

Squashed in commit 6aa0d21

@FlorianHockmann
Copy link
Member

I am able to reproduce the same error. I checked with an older version of the code without the windows build support changes in commit 38a3bd7 on 3.5 and the same error exists there as well.

Shall I document this in TINKERPOP-2760, which I made for pending windows tasks or create a new ticket for it?

If it's not specific to Windows, then I think that a separate ticket makes more sense.

VOTE +1

@L0Lmaker
Copy link
Contributor Author

L0Lmaker commented Jul 6, 2022

If it's not specific to Windows, then I think that a separate ticket makes more sense.

No, it is not specific to Windows, I will create a new ticket for it.

Update: Ticket created
TINKERPOP-2770

@FlorianHockmann FlorianHockmann merged commit 47a855a into apache:3.5-dev Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants