-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fixed windows download #178
Conversation
Codecov Report
@@ Coverage Diff @@
## ign-fuel-tools4 #178 +/- ##
===================================================
+ Coverage 78.07% 78.40% +0.32%
===================================================
Files 19 19
Lines 2636 2676 +40
===================================================
+ Hits 2058 2098 +40
Misses 578 578
Continue to review full report at Codecov.
|
It would be nice to fix it since Citadel, and we can merge forward. Are there any tests that are currently skipped on Windows that we can enable with this PR? grep -r "IGN_UTILS_TEST" .
|
I fixed the But I have a problem with the following folder @chapulina should we remove this symbol ? o replace it for other one ? |
Ohh I see, how about we substitute the symbol when creating the folder just on Windows? So we keep backwards compatibility for Unix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified that download is working on Windows now 👍 I'll leave it up to you if you want to fix the test or leave that to a future PR.
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
69c3f31
to
3b7e3d8
Compare
I retargeted this to |
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
This PR depends on gazebosim/gz-common#197 |
Signed-off-by: ahcorde <[email protected]>
@osrf-jenkins retest this please |
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@osrf-jenkins retest this please |
ou yeah CI is green! @chapulina do you mind to have a look ? |
This seems like an infra problem on the Windows build CC @j-rivero
|
@osrf-jenkins retest this please |
Signed-off-by: ahcorde <[email protected]>
@osrf-jenkins retest this please |
@mjcarroll this build shows that Windows is also green |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that this works with ign-gazebo
's resource spawner on Linux. I just have some nitpicks.
src/FuelClient.cc
Outdated
@@ -986,6 +986,14 @@ bool FuelClient::ParseWorldFileUrl(const common::URI &_fileUrl, | |||
return false; | |||
} | |||
|
|||
std::vector<std::string> tokens = ignition::common::split(file, "/"); | |||
std::string fileTemp; | |||
for (auto s : tokens) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you resolved the conversation but didn't apply the change. Not sure if you think it's not needed or overlooked it?
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Retriggering again Windows build, this PR gazebo-tooling/release-tools#463 on release-tools should fix the issue. |
@osrf-jenkins retest this please |
* Owner upload (#179) Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Louise Poubel <[email protected]> * Fixed windows download (#178) * Fixed donwload on Windows Signed-off-by: ahcorde <[email protected]> * Fixed interface_TEST Signed-off-by: ahcorde <[email protected]> * Improved windows support Signed-off-by: ahcorde <[email protected]> * Make linters happy Signed-off-by: ahcorde <[email protected]> * Make linters happy Signed-off-by: ahcorde <[email protected]> * Improved Signed-off-by: ahcorde <[email protected]> * Fixed test on Windows Signed-off-by: Alejandro Hernández <[email protected]> * Fixed test Signed-off-by: ahcorde <[email protected]> * Fix some nits Signed-off-by: ahcorde <[email protected]> * Improved Windows support Signed-off-by: ahcorde <[email protected]> * Fixed test on Linux Signed-off-by: ahcorde <[email protected]> * make linters happy Signed-off-by: ahcorde <[email protected]> * Fixed windows tests Signed-off-by: ahcorde <[email protected]> * Fixed tests Signed-off-by: ahcorde <[email protected]> * Added feddback Signed-off-by: ahcorde <[email protected]> * make linters happy Signed-off-by: ahcorde <[email protected]> * Remove tools/code_check and update codecov (#187) Signed-off-by: Louise Poubel <[email protected]> * added fuel update command (#185) * added fuel update command Signed-off-by: Tomas Lorente <[email protected]> * fixed header Signed-off-by: Tomas Lorente <[email protected]> * fixed build Signed-off-by: Tomas Lorente <[email protected]> * updated docs Signed-off-by: Tomas Lorente <[email protected]> * added header Signed-off-by: Tomas Lorente <[email protected]> * fix Signed-off-by: Tomas Lorente <[email protected]> * nit2 Signed-off-by: Tomas Lorente <[email protected]> * Style, and headers Signed-off-by: Nate Koenig <[email protected]> * Fixed world download Signed-off-by: Nate Koenig <[email protected]> * Removed debug Signed-off-by: Nate Koenig <[email protected]> * Fix tests Signed-off-by: Nate Koenig <[email protected]> * Fix windows Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> * 🎈 4.4.0 (#190) Signed-off-by: Louise Poubel <[email protected]> * Detect ign instead of using cmake module to check for ignition-tools (#191) Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Louise Poubel <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Jose Tomas Lorente <[email protected]> Co-authored-by: Jose Luis Rivero <[email protected]>
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1 |
Signed-off-by: ahcorde [email protected]
🦟 Bug fix
This PR will allow to download model and world on Windows. I checked the functionality with the executable
download
in the example folder.Should I retarget this a different ign-fuel-tools version ?
Summary
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge