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

joda-convert and joda-time #24

Merged
merged 2 commits into from
Sep 28, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,15 @@ os: osx
# on Traivs.
env:
- TEST_DIR=com.google.code.gson-gson
- TEST_DIR=org.joda-joda-convert
- TEST_DIR=joda-time-joda-time
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that is indeed the ugly fully qualified name for this.

like apache, joda is not consistent with its group id

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked britter about this in the other thread. Let's see if he responds with some insights.

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 mean it is definitely correct:
https://github.com/JodaOrg/joda-time/blob/master/pom.xml#L8

just odd


matrix:
allow_failures:
# Blocked on https://github.com/JodaOrg/joda-convert/issues/7
Copy link
Contributor

Choose a reason for hiding this comment

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

Further expand comment:

# Should be fixed in next release: version 1.8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know that. The next version number is something they'll come up with (maybe 1.7.1, maybe 2.0, etc.), and even that we have no guarantee they will actually release. The link is sufficient - no reason to add possibly wrong info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Namely, this is a snapshot only:
https://github.com/JodaOrg/joda-convert/blob/master/pom.xml#L12

When release time comes, depending on the API changes the maintainer might need to call it 2.0, 1.7.1, etc.

- env: TEST_DIR=org.joda-joda-convert
# Blocked on https://github.com/JodaOrg/joda-convert/issues/7
- env: TEST_DIR=joda-time-joda-time

branches:
only:
Expand Down
32 changes: 31 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,35 @@ You are not required to run the system tests locally before creating a PR (they
time and processing power), however if the tests fail on Travis you will need to update
the PR until they pass.

#### Adding a new library

When you add a new library to build, make sure it is referenced in run-all.sh (for
human contributors) and in .travis.yml (for continuous builds).
human contributors) and in .travis.yml (for continuous builds). If your library has failures
add the env row to the allow_failures section of .travis.yml and document the blockers.

The structure for each library to test is as follows:

```
libraryBuilds/
- common/ - Existing directory with common build config
- dependencyLib1/ - Existing root of a library that newLibrary depends on
- dependencyLib1/ - Existing project for a library that newLibrary depends on
- dependencyLib2/ - Ditto.
- dependencyLib2/ - Ditto.
- newLibrary/ - New directory for the root project
- newLibrary/ - New directory for the library itself
- build.gradle - Build file containing j2objcTranslation directive for newLibrary
- dependencyLib1/ - Soft link to ../dependencyLib1/dependencyLib1/
- dependencyLib2/ - Soft link to ../dependencyLib2/dependencyLib2/
- settings.gradle - 'include' directive for newLibrary and libraries it depends on
Copy link
Contributor

Choose a reason for hiding this comment

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

Put settings.gradle last as that's the outlier while everything else is softlinks. That's also alphabetical ordering as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- build.gradle - Soft link to ../common/build.gradle, contains the preamble
- gradlew - Soft link to ../../j2objc-gradle/gradlew
- local.properties - Soft link to ../common/local.properties
```

Every library newLibrary depends on must have its own similar structure as a top-level project,
as illustrated by dependencyLib1 and 2. Note the two level directory structure above: even if
your library has no dependencies, the build.gradle file for the library must lie 2 directories
below libraryBuilds1/.

TODO: Create a script to setup the above structure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove and convert to issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

45 changes: 0 additions & 45 deletions libraryBuilds/com.google.code.gson-gson/build.gradle

This file was deleted.

1 change: 1 addition & 0 deletions libraryBuilds/com.google.code.gson-gson/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright (c) 2015 the authors of j2objc-common-libs-e2e-test
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

dependencies {
j2objcTranslation 'com.google.code.gson:gson:2.3.1:sources'
}

j2objcConfig {
// package-info.java exists in multiple packages.
filenameCollisionCheck false
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these identical? Or does this prevent the library from being used?

I'm wondering if issues should be filed related to this:

  1. Within collision check, ignore collision when files are identical?
  2. Create a prefixes.properties file for this and other libraries so that they can be fully used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • prefixes.properties doesn't help with filenames.
  • no the library can be used just fine.
  • package-info.java contains no classes, but is valid way of documenting your packages and quite common (for people who actually use javadoc).

Btw this file was just moved from one directory up - you reviewed this in the last PR.

EDIT: You never* need prefixes.properties to resolve a filename collision. Without prefixes.properties remember that each package has a distinct, yet unwieldy, prefix already. You can only cause problems by using --no-package-directories and/or using a non-well-formed prefixes.properties that causes 2 packages with an overlap in classnames to also have the same prefix.

*ok not never, if the suffix of a package path is identical to the prefix of an existing class name, you can still cause collisions. like package.box.Turtle and package.BoxTurtle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also the real fix is j2objc-contrib/j2objc-gradle#467


// Almost always there are no tests provided in an external source jar.
testMinExpectedTests 0
finalConfigure()
}
1 change: 1 addition & 0 deletions libraryBuilds/com.google.code.gson-gson/settings.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include ':com.google.code.gson-gson'
34 changes: 34 additions & 0 deletions libraryBuilds/common/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright (c) 2015 the authors of j2objc-common-libs-e2e-test
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

buildscript {
repositories {
jcenter()
}
dependencies {
// This is the build output of the plugin itself.
classpath fileTree(dir: '../../j2objc-gradle/build/libs', include: ['*.jar'])
}
}

subprojects {
apply plugin: 'java'
apply plugin: 'com.github.j2objccontrib.j2objcgradle'
Copy link
Contributor

Choose a reason for hiding this comment

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

How about reversing ordering, so you apply j2objcgradle first? It's kind of a no-op since j2objcgradle will load the java plugin... but this will make sure that continues to work.

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 don't think that's how people conceptually think of the plugin. you start with a java project and then make it a j2objc project. you don't start with a j2objc project and then make it a java project.


repositories {
jcenter()
}
}
1 change: 1 addition & 0 deletions libraryBuilds/joda-time-joda-time/build.gradle
1 change: 1 addition & 0 deletions libraryBuilds/joda-time-joda-time/gradlew
28 changes: 28 additions & 0 deletions libraryBuilds/joda-time-joda-time/joda-time-joda-time/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright (c) 2015 the authors of j2objc-common-libs-e2e-test
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

dependencies {
compile project(':org.joda-joda-convert')
j2objcTranslation 'joda-time:joda-time:2.8.2:sources'
}

j2objcConfig {
autoConfigureDeps true

// No tests in the sources.jar.
testMinExpectedTests 0
finalConfigure()
}
1 change: 1 addition & 0 deletions libraryBuilds/joda-time-joda-time/local.properties
1 change: 1 addition & 0 deletions libraryBuilds/joda-time-joda-time/org.joda-joda-convert
1 change: 1 addition & 0 deletions libraryBuilds/joda-time-joda-time/settings.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include ':org.joda-joda-convert', ':joda-time-joda-time'
1 change: 1 addition & 0 deletions libraryBuilds/org.joda-joda-convert/build.gradle
1 change: 1 addition & 0 deletions libraryBuilds/org.joda-joda-convert/gradlew
1 change: 1 addition & 0 deletions libraryBuilds/org.joda-joda-convert/local.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (c) 2015 the authors of j2objc-common-libs-e2e-test
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

dependencies {
j2objcTranslation 'org.joda:joda-convert:1.7:sources'
}

j2objcConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing filenameCollisionCheck, which is good ;-)

// No tests in the sources.jar.
testMinExpectedTests 0
finalConfigure()
}
1 change: 1 addition & 0 deletions libraryBuilds/org.joda-joda-convert/settings.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include ':org.joda-joda-convert'
2 changes: 2 additions & 0 deletions libraryBuilds/run-all.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@
set -ev

./run-test.sh com.google.code.gson-gson
./run-test.sh org.joda-joda-convert
./run-test.sh joda-time-joda-time