API review

Proposer: Tully Foote

Present at review:

  • Tully
  • Wim
  • Stu
  • Jeremy
  • Tony

Changes to be reviewed

  • New additive behavior (developed for hackathon use case)
    • If given duplicate local_name, override the previous version.
    • When installing if the URL is different, mv directory to backup path(ROSINSTALL_DIRECTORY/backup/. This allows changes in URLs. Before it just called svn up or equivilent.
    • This allows easy adding, and modifying, but not deletion. (Deletion requires hand editing of .rosinstall file)

Question / concerns / comments

Stu

  • Suppose I re-rosinstall the entire rosinstall file. That's a lot of things to replace, and will result in a large backup directory.
    • Tully It will only backup things which have changed URLs, so if the file is the same it won't backup anything.

Tony

  • What happens if the git branch tag changes? Does it consider it part of the "URL" and back it up? What about other tags that may have similar effects?
    • Tully Yes a changing branch should be considered a changed url. Ticketed <<Ticket(ros 2907)>>

  • What happens if an item is deleted from the .rosinstall file, and you want it to go away in the directory?
    • Tully It will no longer be in your path when generated. It will still be on disk. I do not feel comfortable deleting anything. A manual deletion is required. A ticket is open to warn these directories exist <<Ticket(ros 2572)>>

  • What happens if an item in the .rosinstall file is called "backup"?
  • What happens if the URL for an item changes multiple times?
  • Under this scheme, we have to be careful with the ros package path. If we set it to the ROSINSTALL_DIRECTORY, then packages in the backup might end up in the package path. If this happens, code in those packages could run instead of the updated code.

Jeremy

  • I generally agree that the backup mechanism makes me nervous
    • What gets backed up? The whole rosinstall? Just the portion below the localname which had the url-change?
      • Tully Just the portion below local-name. The goal is to never lose uncommitted changes

    • Tony makes a good point about being sure to hide these from rospack reliably.
  • Not really addressed here, but I still think we need a better mechanism for incremental updates and deletion.
    • It's a pain to always have to retype the full path to your rosinstall in order to do an update rather than rosinstall caching the url for the rosinstall file.
      • Tully Ticket as feature enhancement <<Ticket(ros 2837)>>

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

  • /!\ Proposed change by Wim:

    • Change default behavior to prompt, [Delete, Abort, Backup]
      • Delete deletes the file
      • Abort stops processing
      • backup prompts for directory into which to backup
      • New Command line options: --delete-changed-urls --abort-changed-urls --backup-changed-urls=PATH_TO_BACKUP_INTO
        • Stu: there may be more general names for these. Something more like -f

    • This seems to deal with Tony's comments about avoiding conflicts, and Jeremy's worries about automatic backups
    • There was also an issue of possible ballooning disk space made by multiple backups, which is now a lot clearer to the user.
  • Proposed change to extra directory handling:
    • Instead of warning about extra directories at end prompt user to [Delete, Ignore] extra directories
      • Command line options --ignore-extra-directories or --delete-extra-directories.

Conclusion

Package status change mark change manifest)

  • /!\ Action items that need to be taken.

    • /!\ <<Ticket(ros 2572)>>

    • /!\ <<Ticket(ros 2907)>>

  • {X} Major issues that need to be resolved

    • {X} Change to prompt for backup <<Ticket(ros 2921)>>


Wiki: rosinstall/Reviews/2010-07-08_API_Review (last edited 2010-07-26 20:46:47 by TullyFoote)