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

invert order of package type detection (dry before wet) (ros/rospkg#30) #11

Merged
merged 1 commit into from
Mar 12, 2013

Conversation

dirk-thomas
Copy link
Member

@tfoote @wjwwood Please review and consider scenarios where inverting the precedence could be a problem.

@@ -1369,8 +1374,7 @@ bool cmpDirectoryCrawlRecord(DirectoryCrawlRecord* i,
}
else
{
fs::path manifest_path = fs::path(path) / manifest_name_;
stackage = new Stackage(name, path, manifest_path.string(), manifest_name_);
return;
Copy link
Member

Choose a reason for hiding this comment

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

Is return the correct thing to do here?

It seems like it used to be that if there were neither package.xml nor manifest.xml it would continue:

if package.xml
  process package.xml
else
 process manifest.xml
endif
# continue with code...

But it will return if that is the case:

if manifest.xml
  process manifest.xml
else if package.xml
  process package.xml
else
  return
endif
# continue with code..

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning directly in the case that neither of the files exist seems to be cleaner behavior.
Anyway this should not happen since it is always checked with isStackage() if the path is one of these two types.

@tfoote
Copy link
Member

tfoote commented Mar 12, 2013

If in a wet workspace this won't give link flags because it won't be
queried. Enabling this behavior will prevent the ability to catkin_make
and rosmake the same directory as it will be detected by both, and compiled
for both passes.

On Mon, Mar 11, 2013 at 6:20 PM, Dirk Thomas notifications@github.comwrote:

@tfoote https://github.com/tfoote @wjwwood https://github.com/wjwwoodPlease review and consider scenarios where inverting the precedence could

be a problem.

You can merge this Pull Request by running

git pull https://github.com/ros/rospack dry_wet_order

Or view, comment on, or merge it at:

#11
Commit Summary

File Changes

Patch Links:

@dirk-thomas
Copy link
Member Author

@tfoote rospack will alway return the dry flags for a hybrid package but the result should be equal to the wet information (else the meta information stored in both files is inconsistent). Building a workspace with catkin_make and after that with rosmake should be fine the dry variant should overlay the wet variant as it is built in a overlay on top.

@wjwwood
Copy link
Member

wjwwood commented Mar 12, 2013

I can't think of any way this could break something, but that's not to say it won't. Either way +1

dirk-thomas added a commit that referenced this pull request Mar 12, 2013
invert order of package type detection (dry before wet) (ros-infrastructure/rospkg#30)
@dirk-thomas dirk-thomas merged commit 5688a62 into groovy-devel Mar 12, 2013
@dirk-thomas dirk-thomas deleted the dry_wet_order branch March 12, 2013 20:11
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