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

Revert "create component_container_isolated (#1781)" #1840

Closed
wants to merge 1 commit into from

Conversation

Blast545
Copy link
Contributor

@Blast545 Blast545 commented Dec 8, 2021

Opening this to test if indeed #1781 is causing the windows regression.

This reverts commit 321c74c.

@Blast545
Copy link
Contributor Author

Blast545 commented Dec 8, 2021

Windows CI up to rclcpp_components
Build Status

@Blast545 Blast545 marked this pull request as ready for review December 8, 2021 17:55
@Blast545 Blast545 requested a review from clalancette December 8, 2021 17:57
@SteveMacenski
Copy link
Collaborator

SteveMacenski commented Dec 8, 2021

@Blast545 can you fix the issue rather than simply reverting it? I think the major issue (if my understanding is correct) is that implementations need to be moved into the header or the cpp file needs to create instantiations of each ExecutorT type (e.x. https://github.com/ros-planning/navigation2/blob/main/nav2_smac_planner/src/a_star.cpp#L426-L429). Probably the first is best since these functions are so simple.

(at least that's what I can ascertain from Googling those Windows errors as to analogs in Linux development)

@clalancette
Copy link
Contributor

@Blast545 can you fix the issue rather than simply reverting it?

Blash545's role is generally in keeping the buildfarm green. If the fix is easy enough, that can mean fixing it forward, but if it is involved it generally means a revert.

That is to say, someone involved with the original PR (or at least involved with rclcpp) really needs to look at it and fix it. I would generally jump on these things, but I don't really have time for the rest of this week, and we can't leave Windows broken for days. So if @ivanpauno or @gezp can't look at it today, I think a revert is in order.

@SteveMacenski
Copy link
Collaborator

SteveMacenski commented Dec 8, 2021

Fair enough. I don't have a Windows environment to test in (nor have I ever developed in Windows either), so what I said above would be my best guess as to the solution, but I couldn't know until running CI. I can submit a PR in a couple of hours for this if that would be helpful -- else we can wait for @gezp / revert.

Let me know what you want me to do.

@clalancette
Copy link
Contributor

Fair enough. I don't have a Windows environment to test in (nor have I ever developed in Windows either), so what I said above would be my best guess as to the solution, but I couldn't know until running CI. I can submit a PR in a couple of hours for this if that would be helpful -- else we can wait for @gezp / revert.

That's fine. I would say we can go one more day with this, so let's wait for @gezp . If we don't have a fix forward by tomorrow morning Pacific time, we'll do a revert.

@gezp
Copy link
Contributor

gezp commented Dec 9, 2021

sorry, it seems my fault, I also don't have a Windows environment to test , but i will try my best to solve this issue.

I think the major issue (if my understanding is correct) is that implementations need to be moved into the header or the cpp file needs to create instantiations of each ExecutorT type.

agree, i think i can try this solution, i will make a PR latter.

@gezp
Copy link
Contributor

gezp commented Dec 9, 2021

i think i solved this issue, please see more details here #1843, i have tested on my local win10 environment, it could be built successfully.

@clalancette
Copy link
Contributor

Closing, as this was fixed forward by #1843

@clalancette clalancette closed this Dec 9, 2021
@clalancette clalancette deleted the blast545/revert_rclcpp_PR_1871 branch October 12, 2022 19:34
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