Skip to content

changed gmapping source repo, adapted patch#80

Merged
2maz merged 2 commits intorock-core:masterfrom
leifole:master
Oct 22, 2015
Merged

changed gmapping source repo, adapted patch#80
2maz merged 2 commits intorock-core:masterfrom
leifole:master

Conversation

@leifole
Copy link
Contributor

@leifole leifole commented Aug 28, 2015

the source for gmapping that is used in ROS has some (a lot of) fixes, that are not in the original openslam repo, so adapted source.yml to that. Also changed the patch to remove catkin-related stuff. This time used --no-prefix with git format-patch so default patchlevel -p0 works.

@goldhoorn
Copy link
Contributor

Would be great if you can took-over the maintainer ship for this package please:

  1. add a manifest.xml
  2. make sure autoproj behaves correctly on this repro change. i don't know yet the state of autoproj what happens if you simply change the type of a package.
    Autoproj should not rause a error if you NOT delete the old-gmapping before (similar to: use opencv's git instead of archive package_set#14)

@leifole
Copy link
Contributor Author

leifole commented Sep 1, 2015

Indeed, autoproj is failing if you change the repository type, without deleting the local working copy first:

autoproj: importing and loading selected packages
  patching slam/gmapping: unapplying 1 patch(es)
  updated slam/gmapping         
Command failed             
slam/gmapping(/home/leifole/dev/rock/transterra/slam/gmapping): failed in import phase
    while importing slam/gmapping, /home/leifole/dev/rock/transterra/slam/gmapping does not point to a git repository

How to proceed? Should be possible to change repo type for existing packages. Since I am no big fan of magic behind the scenes, I would suggest just asking the user to manually delete the old folder.

@doudou
Copy link
Member

doudou commented Sep 1, 2015

How to proceed? Should be possible to change repo type for existing packages. Since I am no big fan of magic behind the scenes, I would suggest just asking the user to manually delete the old folder.

Ideally, that's what autobuild would do. Never got implemented. Feel free.

@doudou
Copy link
Member

doudou commented Sep 1, 2015

(On a more constructive note: you can IMO change it and tell people on the ML that they'll have to delete the folder)

@leifole
Copy link
Contributor Author

leifole commented Sep 11, 2015

autobuild is raising the ConfigException, for example in import/git.rb . Most simple thing to do would be perhaps to just extend the exception message, either in all files under import/ or in the ConfigException definition in exceptions.rb.

Further information (like "expected git, but found #{repo_type}, source switched? -> Delete first") would be nice, but I'm not sure, where the best place would b to check for the right vcs type.

@doudou
Copy link
Member

doudou commented Sep 11, 2015

Further information (like "expected git, but found #{repo_type}, source switched? -> Delete first") would be nice, but I'm not sure, where the best place would b to check for the right vcs type.

There's no awesome way to robustly detect the source type, and I'm not entirely sure that it's really that useful information. We can definitely extend the message by saying that the importer has changed and that the user might want to delete the source first, though.

@2maz
Copy link
Member

2maz commented Oct 16, 2015

Related pull requests now: #91 and rock-core/package_set#14

As from what I see there is no way and need to handle this automagically, but requires informing the users. So notification on the ML should do until we properly fix it in autoproj.

Pull request to update the error message on git import: rock-core/autobuild#30

2maz added a commit that referenced this pull request Oct 22, 2015
changed gmapping source repo, adapted patch
@2maz 2maz merged commit d9e45bb into rock-core:master Oct 22, 2015
@2maz
Copy link
Member

2maz commented Oct 22, 2015

Merged after announcement on user ML.

Rauldg pushed a commit to Rauldg/rock-package_set that referenced this pull request Aug 16, 2019
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.

4 participants