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

Add CocoaPods dependency manager #3994

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

pietbrauer
Copy link
Contributor

@pietbrauer pietbrauer commented May 7, 2021

analyzer: Add initial support for the CocoaPods dependency manager

Constructing the dependency tree for any given Podfile solely from the
output of the pod sub-commands requires running pod sub-commands
which only run on macOS. This implementation avoids all macOS-only
commands by constructing the dependency tree from the lockfile and
therefore the analysis of any Podfile requires the precence of the
corresponding Podfile.lock.

In order to obtain the meta-data for a dependency, the corresponding
.podspec file is determined via the command pod spec which. These
paths always point under ~/cocoapods/repos which is where CocoaPods
sets up its so called Specs repositories. Which repositories are to be
used is defined by the respective Podfile via the source property,
which defaults to [1]. This implementation disregards the specified
repositories and always assumes that default. That default specs
repository is however not used via GitHub, but via CDN as this is much
faster [2]. Cloning [1] takes 10-30 minutes and takes a bit more than
5GB while the CDN approach initializes in no time and fetches only
required data.

Even with that CDN based approach the implementation can
and should be extended to adhere to the specified repositories in the
future, see #4188.

[1] https://github.com/CocoaPods/Specs
[2] CocoaPods/CocoaPods#7046

------------------------------------- Original description from Piet below -----------------------------------

This PR adds CocoaPods as a dependency manager.

I tested it on macOS and inside the docker container. The Gem runs fine on Linux, the only thing that doesn't work is pod install because Xcode is missing. As far as I am concerned pod install is not needed, if the Podfile.lock is present.

Things I am unsatisfied with/didn't know better:

  1. How to mock the Command invocations to test the actual flow
  2. JSON parsing
  3. Parsing the strings like Alamofire/Something (~> 1.0) into a PackageReference
  4. I also don't like cloning the spec repo inside the docker container which takes about 5 Minutes and adds a lot of storage. Setting it up on demand is even worse, if you do it every time you launch the docker container.

Any pointers are welcome.

@sschuberth
Copy link
Member

Thanks for the contribution! Unfortunately, at least I'd need a few more days before I could start looking at this.

@fviernau
Copy link
Member

fviernau commented Jun 4, 2021

Hi @pietbrauer ,

thanks for your contribution! I'm picking up this review now - mind helping me with the below initial questions?

  1. You mentioned "As far as I am concerned pod install is not needed, if the Podfile.lock is present.". Can you give me a hint
    why install is necessary if there is no lock file?
  2. Regarding "I also don't like cloning the spec repo inside the docker container which takes about 5 Minutes and adds a lot of storage. Setting it up on demand is even worse, if you do it every time you launch the docker container." Have you considered the approach to not clone it initially but on-the-fly, and at the same time recommend the user to either (a) bind mount a host directory into the Docker or (b) use a Docker volume for that directory?
  3. Have there been any higher level implementation / design decisions which you made, which aren't obvious from the PR description or commit (e.g. like choosing whether to parse lock files)? If so, mind sharing them e.g. in the PR description?
  4. Do you know whether it's possible to make install work on linux?
  5. Does the implementation have any known limitations, things it cannot handle.

@fviernau
Copy link
Member

fviernau commented Jun 4, 2021

@pietbrauer In order move a bit faster, I've setup a branch [1] in this repository and pushed your changes (rebased onto master). I'll make some modifications to the code there (force push) and as soon as done we may create a new PR from that branch replacing this PR. Fine with you?

[1] https://github.com/oss-review-toolkit/ort/tree/cocoa-pods-support

@pietbrauer
Copy link
Contributor Author

@fviernau I'm currently on parental leave and won't work on it until August. We have it in use at Porsche so we don't have time pressure.

I don't get the working on a different branch but if that's how you want to review it so be it.
I enabled that maintainers can push onto this branch so rebasing and committing should be possible without splitting all the work onto multiple PRs or branches I can't push to.

@fviernau fviernau force-pushed the cocoapods branch 3 times, most recently from 8fb9464 to e22946e Compare June 15, 2021 13:02
@fviernau
Copy link
Member

fviernau commented Jun 15, 2021

@pietbrauer @sschuberth @mnonnenmacher

I refactored or re-wrote a couple of things mainly for simplification / clarity.
Therefore I

  • dropped the previous unit tests in favor of functional ones, as I needed to get rid of implementation detail tests in
    order to enable the refactoring. The functional tests cover the cases Piet had covered before.
  • I've addressed the points 2, 3 mentioned by Piet in the description.
    For 3, parsing version string of the form Alamofire/Something (~> 1.0) is not needed anymore as versions
    are now only taken from top level entries which never use that format including the tilde.
  • Point 4 is addressed by not using the Github specs repo anymore, but the CDN repo.

I'd see Point 1 not as an issue anymore and I'd prefer to add the documentation in a follow up PR.

edit: I'm still working on the CI issues, so only the Kotlin bits are ready.

@fviernau fviernau force-pushed the cocoapods branch 2 times, most recently from 0b631da to bc2174d Compare June 15, 2021 14:32
Copy link
Member

@mnonnenmacher mnonnenmacher left a comment

Choose a reason for hiding this comment

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

Please also update the list of supported package managers in the README.

reporter-web-app/yarn.lock Outdated Show resolved Hide resolved
.azure-pipelines/LinuxAnalyzerTest.yml Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
@fviernau fviernau force-pushed the cocoapods branch 10 times, most recently from b96ba35 to 77646be Compare June 17, 2021 17:59
mnonnenmacher
mnonnenmacher previously approved these changes Jun 18, 2021
mnonnenmacher
mnonnenmacher previously approved these changes Jun 18, 2021
.azure-pipelines/LinuxAnalyzerTest.yml Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
analyzer/src/main/kotlin/PackageManager.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/CocoaPods.kt Outdated Show resolved Hide resolved
pietbrauer and others added 2 commits June 22, 2021 12:09
Constructing the dependency tree for any given `Podfile` solely from the
output of the `pod` sub-commands requires running `pod` sub-commands
which only run on macOS. This implementation avoids all macOS-only
commands by constructing the dependency tree from the lockfile and
therefore the analysis of any `Podfile` requires the precence of the
corresponding `Podfile.lock`.

In order to obtain the meta-data for a dependency, the corresponding
`.podspec` file is determined via the command `pod spec which`. These
paths always point under `~/cocoapods/repos` which is where CocoaPods
sets up its so called Specs repositories. Which repositories are to be
used is defined by the respective `Podfile` via the `source` property,
which defaults to [1]. This implementation disregards the specified
repositories and always assumes that default. That default specs
repository is however not used via GitHub, but via CDN as this is much
faster [2]. Cloning [1] takes 10-30 minutes and takes a bit more than
5GB while the CDN approach initializes in no time and fetches only
required data.

Even with that CDN based approach the implementation can
and should be extended to adhere to the specified repositories in the
future.

The CocoaPods repository cannot be checked out on Windows due to special
characters in a path. So, installation is not possible on Windows which
is why the tests are disabled on Windows for now.

[1] https://github.com/CocoaPods/Specs
[2] CocoaPods/CocoaPods#7046

Co-authored-by: Frank Viernau <[email protected]>
Signed-off-by: Piet Brauer-Kallenberg <[email protected]>
Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau merged commit 5bf32d6 into oss-review-toolkit:master Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants