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

Vehicle installation requirements (part 3) [RDY] #17302

Merged
merged 12 commits into from
Jun 23, 2016

Conversation

mugling
Copy link
Contributor

@mugling mugling commented Jun 22, 2016

Refactor veh_interact.cpp with no behavioral changes

@mugling mugling force-pushed the veh18 branch 4 times, most recently from 816c737 to 4952b67 Compare June 22, 2016 11:00
@mugling mugling changed the title Vehicle installation requirements (part 3) [WIP] Vehicle installation requirements (part 3) [RDY] Jun 22, 2016
@Coolthulhu
Copy link
Contributor

You could also fix #17314 here and change one of the "You don't have requirements" that was left out in the last PR.

@Coolthulhu Coolthulhu self-assigned this Jun 23, 2016
@Coolthulhu
Copy link
Contributor

Managed to get a crash.
I tried to install a cargo carrier on a car chassis. I turned on debug hammerspace and tried installing it while spawning extra tools. After few tries, I got a bunch of debugmsgs relating to failed install and a segfault.

@mugling
Copy link
Contributor Author

mugling commented Jun 23, 2016

Let me take a look

@mugling
Copy link
Contributor Author

mugling commented Jun 23, 2016

#17314 can be addressed via #17324 which provides an ideal mechanism to ensure all installation requirements are consistent

@mugling
Copy link
Contributor Author

mugling commented Jun 23, 2016

Managed to get a crash.

Does it occur only with DEBUG_HS?

EDIT: And what debugmsg?

EDIT: Regression or present in master?

@Coolthulhu
Copy link
Contributor

Does it occur only with DEBUG_HS?

I think yes.
But fixing it would be easy - just making the function break rather than continuing when base.is_null().

@Coolthulhu
Copy link
Contributor

#17324 isn't ready to be the proper solution yet. It would be better to fix #17314 in the simplest way at the moment.

That "you lack the requirements" - it's just weird phrasing, but it's trivial to fix.

@mugling
Copy link
Contributor Author

mugling commented Jun 23, 2016

Ok both pushed

@Coolthulhu Coolthulhu merged commit ebdd482 into CleverRaven:master Jun 23, 2016
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.

2 participants