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

👩‍🌾 Fix Windows build #103

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Conversation

chapulina
Copy link
Contributor

The Windows build was broken on #98.

The problem was using a class that doesn't have a default constructor inside a future:

https://github.com/ignitionrobotics/ign-fuel-tools/blob/2eb0ee784c0c795289dd34e64ff2b814f9cc64c8/src/ign.cc#L611

I tried a few different things but ended up just giving the class a default constructor, which I don't think is unreasonable.

@chapulina chapulina requested a review from azeey August 18, 2020 22:02
@chapulina chapulina requested a review from nkoenig as a code owner August 18, 2020 22:02
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Aug 18, 2020
@chapulina chapulina added bug Something isn't working Windows Windows support and removed 📜 blueprint Ignition Blueprint labels Aug 18, 2020
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #103 into ign-fuel-tools3 will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           ign-fuel-tools3     #103      +/-   ##
===================================================
+ Coverage            86.04%   86.08%   +0.03%     
===================================================
  Files                   18       19       +1     
  Lines                 2215     2221       +6     
===================================================
+ Hits                  1906     1912       +6     
  Misses                 309      309              
Impacted Files Coverage Δ
include/ignition/fuel_tools/Result.hh 100.00% <ø> (ø)
src/Result.cc 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eb0ee7...1e62488. Read the comment docs.

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM! Didn't know about std::future needing a type with a default ctor.

@@ -164,3 +164,10 @@ TEST(CollectionIdentifier, AsPrettyString)
EXPECT_NE(str.find("raspberry"), std::string::npos);
}
}

//////////////////////////////////////////////////
int main(int argc, char **argv)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is it necessary to have the main here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it was just one of the things I tried. It follows the same pattern as ModelIdentifier and WorldIdentifier

@chapulina chapulina merged commit 1b73894 into ign-fuel-tools3 Aug 18, 2020
@chapulina chapulina deleted the chapulina/windows_fail branch August 18, 2020 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants