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

Set dependencies from git #319

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

andrea689
Copy link
Collaborator

@tobrun @m0nac0 What do you think if we set the dependencies from git? In this way anyone can use the master branch in their projects without problems even if mapbox_gl_platform_interface and mapbox_gl_web are not yet published.

@m0nac0
Copy link
Collaborator

m0nac0 commented Jun 9, 2020

@andrea689 Could you please change the URLs to the https URLs? The git@github.com URLs only work with ssh keys.

@m0nac0
Copy link
Collaborator

m0nac0 commented Jun 9, 2020

In general, I haven't tested this, but I guess if someone has a git depedency on mapbox_gl but hasn't cloned the repo, our current approach with the local path dependencies will fail. So I think git dependencies are indeed the better solution.

@andrea689
Copy link
Collaborator Author

@m0nac0 urls updated!
in fact I have tried to use:

mapbox_gl:
    git:
      url: git@github.com:tobrun/flutter-mapbox-gl.git
      ref: master

and I had a local dependencies error

m0nac0 referenced this pull request Jun 9, 2020
* add multiple maps support
* fix ScrollingMapPage example
* use local dependencies
@kleeb
Copy link
Contributor

kleeb commented Jun 9, 2020

+1 for merging, I am also using the master ref

@kleeb
Copy link
Contributor

kleeb commented Jun 9, 2020

still getting on this branch

Error on line 20, column 11: Invalid description in the "mapbox_gl_web" pubspec on the "mapbox_gl_platform_interface" dependency: "../mapbox_gl_platform_interface" is a relative path, but this isn't a local pubspec.
   ╷                                                                    
20 │     path: ../mapbox_gl_platform_interface                          
   │           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                          
   ╵  

@kleeb
Copy link
Contributor

kleeb commented Jun 9, 2020

pubspec.lock has not been updated

@m0nac0
Copy link
Collaborator

m0nac0 commented Jun 9, 2020

EDIT: Please forget everything I wrote. I totally misinterpreted pub's error message. The error means that pub tries to download master from the repo, but the pubspec.yaml on master still has the old "../mapbox_gl_platform_interface" dependency as this PR hasn't landed, of course. I just tested it in my fork and everything should be fine after this PR lands.


@kleeb Deleting pubspec.lock doesn't solve the error for me, unfortunately.
@andrea689 Judging from the errors, pub doesn't seem to allow git: path: or really any path: dependencies in referenced plugins. If that's true, this approach won't work. I guess we need to discuss how we want to manage these dependencies in the future. Possibilities I can think of:

  1. Keep the status quo (using path: dependencies, depend on the published versions only in release branches):
    => users can't use git: dependencies on this repo and will have to clone instead.
  2. Move web and platform_interface to separate repos and change the main repo pubspec to use git: dependencies on these separate repos:
    => will make development much more difficult
  3. Depend on the published web and platform_interface versions:
    => we have to manually change the dependencies to path: everytime we develop a feature and git: dependencies on this repo still won't work when we have unpublished changes to web and platform_interface on master (probably most of the time)
  4. ?? Ask flutter devs why git: path: dependencies aren't allowed in a "chain" of git dependencies (It makes sense to not allow path: dependencies in remote repos, but git: path: should work just fine everywhere git: works IMO)

What do you guys think / do you have more ideas?

@kleeb
Copy link
Contributor

kleeb commented Jun 9, 2020

ad. 2 seems valid approach, doesn't feel like too much hassling when you keep feature branches synced

@m0nac0
Copy link
Collaborator

m0nac0 commented Jun 9, 2020

EDIT: I was wrong, see above


For cases 1 and 3, git dependencies on this repo should work again if this:

 dependency_overrides: 
     mapbox_gl_platform_interface:
      git:
        url: https://github.com/tobrun/flutter-mapbox-gl.git
        path: mapbox_gl_platform_interface
     mapbox_gl_web:
       git:
         url: https://github.com/tobrun/flutter-mapbox-gl.git
         path: mapbox_gl_web

is added to the pubspec.yaml of the app using the mapbox_gl plugin. I haven't tested this, yet, but we have something very similar:
https://github.com/tobrun/flutter-mapbox-gl/blob/1d4cde003bced470861b33e17fef84fe7e85d0c5/example/pubspec.yaml#L18-L22
on master for the example app and it works fine.

@andrea689 andrea689 merged commit 6a83311 into flutter-mapbox-gl:master Jun 9, 2020
@kleeb
Copy link
Contributor

kleeb commented Jun 9, 2020

@m0nac0 great idea, you were super close
this one works like charm:

dependency_overrides:
  mapbox_gl_platform_interface:
    git:
      url: https://github.com/tobrun/flutter-mapbox-gl.git
      path: mapbox_gl_platform_interface
      ref: 6a83311a960975597ac2cb6cd647378509cbf1e2
  mapbox_gl_web:
    git:
      url: https://github.com/tobrun/flutter-mapbox-gl.git
      path: mapbox_gl_web
      ref: 6a83311a960975597ac2cb6cd647378509cbf1e2

@andrea689
Copy link
Collaborator Author

andrea689 commented Jun 9, 2020

I merged it in master and it is worked on my work project with this:

mapbox_gl:
    git:
      url: https://github.com/tobrun/flutter-mapbox-gl.git

only if i want use my local version use:

dependency_overrides:
  mapbox_gl:
    path: ../flutter-mapbox-gl
  mapbox_gl_platform_interface:
    path: ../flutter-mapbox-gl/mapbox_gl_platform_interface
  mapbox_gl_web:
    path: ../flutter-mapbox-gl/mapbox_gl_web

kleeb added a commit to kleeb/flutter-mapbox-gl that referenced this pull request Jun 9, 2020
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.

3 participants