Skip to content
This repository has been archived by the owner on Jan 20, 2025. It is now read-only.

Convert Ant project to Gradle #47

Merged
merged 7 commits into from
Jul 21, 2016
Merged

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Jul 7, 2016

Fixes #46.

This was done using the Android Studio importer and some manual fixing.

The project can now be opened directly with Android Studio, without the need to import anything. It should make contributions easier.

Important notes:

  • The MemorizingTrustManager is now loaded as an external Gradle dependency through Jitpack (https://jitpack.io/). The submodule has been removed.
  • Because the MemorizingTrustManager declares a minSdkVersion of 7, the minSdkVersion of this app has also been changed from 3 to 7.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 7, 2016

Until the last commit, the test failed because Gradle considers linter warnings to be errors by default.

The current warnings are about missing translations. What is your policy? Do you want translations always to be complete (accessibility), or is it OK to keep them incomplete (ease of contributions)?

The latter would probably be more pragmatic. I disabled the lint for now. If you think we should keep it, I can undo that commit.

@rorist
Copy link
Member

rorist commented Jul 8, 2016

Hi, thanks a lot for the contribution, this looks nice.

I didn't realize there were missing translations, forgot about this. It would be better not to disable the warning, so we can be reminded this is an issue we have to address. I'd prefer you revert the last commit, I've created a separate issue #48.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 8, 2016

Ok, will do!

How do we handle contributions where the contributor adds new strings, but does not know the languages involved? Right now, the pull request would be failing in that case due to missing translations.

In any case, even if we allow missing translations in the test suite, creating a release build is still denied if translations are missing.

I'll remove the commit. If you changed your mind I can also re-add it :)

@rorist
Copy link
Member

rorist commented Jul 8, 2016

I'll ask the people who contributed to the initial translation if they can do it.

I think it's not a big deal in our app because usually people in hackerspaces understand english pretty well. But it would be good if there isn't missing translation in the next release, you're right. :)

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 8, 2016

I think it's not a big deal in our app because usually people in hackerspaces understand english pretty well. But it would be good if there isn't missing translation in the next release, you're right. :)

Yes exactly, that's why I'd make it possible to successfully run the tests even if translations are missing, but to deny creating a release with missing translations (the default behavior, at least in Android Studio). These translations could also be provided as separate pull requests.

If you agree with that I'll re-add the commit, ok?

@rorist
Copy link
Member

rorist commented Jul 8, 2016

We don't need to create the release right now as there is no new feature visible to the user. I'd prefer we don't hide this issue in the CI so we can think about it next time, when this is addressed we can then create a new release.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 8, 2016

OK! :)

@rorist
Copy link
Member

rorist commented Jul 8, 2016

I have an other question, why using jitpack ? i'm not familiar with this, but what was wrong with the submodule ? I see there now are commited files of the libs.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 8, 2016

I have an other question, why using jitpack?

It would make dependency management easier. But in the meantime, I changed my mind, because a Gradle dependency without hash verification (e.g. using https://github.com/WhisperSystems/gradle-witness) is vulnerable against MITM attacks. As long as we only have a single dependency, submodules are perfectly fine.

I originally planned to fix that as soon as the branch is merged, but I can do it in this branch as well!

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 8, 2016

I see there now are commited files of the libs.

Wow, that was totally not on purpose. Will fix it, thanks for noting!

dbrgn added 3 commits July 8, 2016 12:17
This was done using the Android Studio importer and some manual fixing.

The project can now be opened directly with Android Studio, without the
need to import anything.

Important notes:

- The MemorizingTrustManager is now loaded as an external Gradle
  dependency through Jitpack (https://jitpack.io/). The submodule has
  been removed.
- Because the MemorizingTrustManager declares a minSdkVersion of 7, the
  minSdkVersion of this app has also been changed from 3 to 7.
@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 8, 2016

Re-added the submodule and renamed the myHackerspace subproject to app (less confusing).

If you want me to squash those last two commits with the first one, let me know.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 20, 2016

What's the status here? :)

@rorist
Copy link
Member

rorist commented Jul 20, 2016

I can't find how to build, can you provide some instructions on the README what to run to build (basically what's in the CI script + setup sdk).

Also the local.proprerties-example was good as you would need to copy it and provide a path to your local sdk copy isn't it ?

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 20, 2016

Thanks for noting, I forgot about the build instructions. README is updated.

As far as I know, the Gradle Android plugin should auto-detect the SDK location. Could you test?

@rorist
Copy link
Member

rorist commented Jul 21, 2016

It's not working for me without the local.properties. Do you have the env variable ANDROID_HOME set by any chance ?

A problem occurred configuring project ':MemorizingTrustManager'.
> SDK location not found. Define location with sdk.dir in the local.properties file or with an ANDROID_HOME environment variable.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 21, 2016

Not that I know of. Maybe Android Studio automatically creates that local.properties file...

I re-added the properties example file.

@rorist rorist merged commit d153b8d into fixme-lausanne:master Jul 21, 2016
@rorist
Copy link
Member

rorist commented Jul 21, 2016

Cool perfect, thank you very much :)

@dbrgn
Copy link
Contributor Author

dbrgn commented Jul 21, 2016

Great, thanks for merging :)

@dbrgn dbrgn deleted the gradle branch July 21, 2016 11:30
cyroxx referenced this pull request in cyroxx/MyHackerspace Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build system
2 participants