[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 So in another words it is a matter of setting up the level of breakage you want to allow between different Tinkerpop versions. I based the "minor increase allows breaking changes" on the "feeling" I got from how Tinkerpop changed over time, but you may want to modify it if you so choose. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 @metlos now that 3.2.4/3.1.6 have been released i thought i'd try to get the TINKERPOP-1443 branch ready for a final review before merging to master. i'm not sure if i'm doing something wrong, but i can't seem to get revapi to fail on build. i thought it would fail immediately when i did a `mvn clean install -DskipTests` but it seems to build without error. I even added an API change to a class in one of the packages listed in api-contents.json, but even then it didn't fail. Am I misinterpreting what I'm seeing somehow or just doing something wrong? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 I've set up the branch internal to TinkerPop: https://github.com/apache/tinkerpop/tree/TINKERPOP-1443 @dkuppitz I tried to apply your patch but it still wouldn't build for me. Not sure if I did something wrong. Would you mind applying it yourself and pushing the fix when you get a minute? @metlos Feel free to close this PR at this point. If you have future updates you can submit PRs against that branch (until it's merged). Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 Hmm, maven central seems to be taking its time - it still doesn't have the latest revapi-java version available (which is why CI failed for the latest commit). But anyway, I've updated the versions to the latest, so you can move on to the internal branch. Let's hope maven central gets synced soon. Thanks! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 Ok - I'm in no rush. Let's wait until you get the new release available before I go to make an internal branch. Thanks --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 I'm near a new release of Revapi that contains some important fixes (like wrong classification of a return type change to a covariant type). So everything I wanted to do to this PR was to update it with the latest Revapi version before it's merged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 Thanks @dkuppitz - I wonder if the easiest thing to do right now is to merge this work into a TinkerPop branch, make some edits so that everything is working how we want, then re-submit the whole thing as a fresh PR. @metlos do you see any problems with that approach? was there anything else you wanted to do to this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/494
This is required to make it work:
```
daniel@cube /projects/apache/tinkerpop (pr-494) $ git diff
diff --git a/docker/build/Dockerfile.template
b/docker/build/Dockerfile.template
index 8364884..d32aa46 100644
--- a/docker/build/Dockerfile.template
+++ b/docker/build/Dockerfile.template
@@ -21,3 +21,7 @@ RUN mkdir -p /usr/src/tinkerpop
RUN curl -s https://bootstrap.pypa.io/ez_setup.py | python
WORKDIR /usr/src/tinkerpop
COPY . /usr/src/tinkerpop
+RUN chmod 744 ./.travis.install-maven.sh
+ENV M2_HOME /root/mvn-home
+ENV PATH ${M2_HOME}/bin:${PATH}
+RUN ./.travis.install-maven.sh 3.3.9 ${M2_HOME}
```
However, this should only go into `tp32`. In `master/` we should make the
maven installation part of the base image.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 @dkuppitz note my previous comment on docker wrt this PR. do we need to alter something in Docker to get this building there? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 Note that `docker/build.sh` doesn't work under on this PR. I assume we need to change the maven version in there too? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 I sent an email to the dev list for consensus on requiring 3.2.5: https://lists.apache.org/thread.html/9ff6c4015d7c57a187d3b66dee46dbf23797483d9bbc11f0304924a6@%3Cdev.tinkerpop.apache.org%3E --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 @okram the API changes seem to be due to your changes in 05bfb029. You could try and fill out your first intentional API changes list ;-) (Most of the below is just git extravaganza - the important bits are points 5, 6 and 7) 1. `git fetch <>` 1. `git checkout -b metlos-api-check-tp32 <>/tp32` 1. `git pull https://github.com/metlos/tinkerpop api-check-tp32` 1. `mvn clean install -DskipTests` 1. copy and paste the JSON snippet output to the build console by revapi into `gremlin-core/api-changes.json`. 1. Follow the example in that file to ignore the found changes in `3.2.4` using the JSON snippet from the build. 1. Fill out the justifications for the changes in that snippet (making it a valid JSON). 1. `git push ssh://g...@github.com/metlos/tinkerpop.git HEAD:api-check-tp32` 1. Watch this PR go green. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/494 I would ignore sub-packages right now. Again, lets start off small, get things passing, and over time, add more packages to check. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 > org.apache.tinkerpop.gremlin.structure > org.apache.tinkerpop.gremlin.structure.io > org.apache.tinkerpop.gremlin.process.computer > org.apache.tinkerpop.gremlin.process.traversal > org.apache.tinkerpop.gremlin.process.traversal.dsl.graph Is this the full list of packages or should the sub-packages of these be included, too? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/494 I'm on Apache Maven 3.3.9 (latest). So I'm ok with downgrading :trollface: --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 i'm still on 3.0.5 - i guess i don't mind upgrading, but we should see what others think (if anything). Do we even want to force an upgrade for TinkerPop 3.2.x? maybe this should go to master for 3.3.0? i guess i'll wait a day for any comments here and then start a thread on dev to assure consensus. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 `.travis.install-maven.sh` is a workaround I used in another project for https://github.com/travis-ci/travis-ci/issues/4872. Trusty, which is the distro used on Travis for Tinkerpop builds, provides Maven 3.1.1, which is too old for Revapi which needs at least 3.2.5. I will push another commit with the changes necessary to track the ignored (aka intentional) API changes. It will be similar to http://revapi.org/modules/revapi-maven-plugin/examples/multi-file-configuration.html. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/494 I didn't dig into this too much just yet - I did see this in the travis logs: ```text If you want to explicitly ignore this change and provide a justification for it, add the following JSON snippet to your Revapi configuration under "revapi.ignore" path: ``` @metlos do you happen to know offhand if this means that if we add a breaking change of some sort that the `revapi.ignore` will just need to continue to fill with these exceptions we have to add? Can those be added to an external file somehow or do they have to be in the pom.xml? also - what's the reason for `.travis.install-maven.sh`? do we need a specific version of maven (3.3.9) for this to work? I generally agree with the listing of initial packages to protect that @okram provided - i'm not sure if adding those will allow travis to pass, but I think we'd want to get the build working in full before merging this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/494 I think we should start this merge off by having only the following packages be analyzed: ``` org.apache.tinkerpop.gremlin.structure org.apache.tinkerpop.gremlin.structure.io org.apache.tinkerpop.gremlin.process.computer org.apache.tinkerpop.gremlin.process.traversal org.apache.tinkerpop.gremlin.process.traversal.dsl.graph ``` Those are the true core concepts. As we get comfortable with this tool and smarter about packaging we can add more things. Thoughts? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/494 > the `ImmutablePath` method changes are internal changes I think that's the point. It would have been an internal change if the methods were internal, but they're public. We can assume that people never used it directly, but we won't know. Maybe someone out there added a TinkerPop dependency in his project, because he liked the `ImmutablePath` implementation, but nothing else (you know, like "let's add Apache Commons, they have a nice method in `StringUtils`"). Hence, if we need to touch public method in a public class: deprecate them and remove/change them in the next major release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 Yes, Java makes it somewhat difficult to properly declare "module boundaries" - let's hope Java 9 will do that right. But for now, it is often useful to declare certain packages as internal and not consider any changes in them as "user-facing". As shown with `org.apache.tinkerpop.shaded`, it is quite easy to exclude packages from the API check with Revapi. If the internal and API classes are mixed within a single package, we could also annotate the internal classes with some dedicated annotation - let's say `org.apache.tinkerpop.Internal` and configure Revapi to ignore anything annotated with that annotation. Revapi can also be configured to just ignore certain classes but that's slightly more involved, because you'd need to modify the POM each time you add an internal class (as opposed to just annotating the class). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/494 I think we will need to decide what is considered NOT subject to change and what is subject to change. For example, the `ImmutablePath` method changes are internal changes that are "okay." Perhaps we can come up with a list of what packages we CAN'T change on minor releases? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #494: TINKERPOP-1443 - Introduce API check into the build
Github user metlos commented on the issue: https://github.com/apache/tinkerpop/pull/494 K, finally the CI fails for the right reasons :wink: https://travis-ci.org/apache/tinkerpop/builds/176007319#L3026 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
