6 ➡️ 7 (main)

[mjcarroll]

➡️ Forward port

Port ign-fuel-tools6 to main

Branch comparison: main...ign-fuel-tools6

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

[codecov]

Codecov Report

Merging #201 (82ae2f7) into main (5e63428) will decrease coverage by 1.91%.
The diff coverage is 25.71%.

❗ Current head 82ae2f7 differs from pull request most recent head d2af0da. Consider uploading reports for the commit d2af0da to get more accurate results

@@            Coverage Diff             @@
##             main     #201      +/-   ##
==========================================
- Coverage   77.47%   75.55%   -1.92%     
==========================================
  Files          20       20              
  Lines        2672     2757      +85     
==========================================
+ Hits         2070     2083      +13     
- Misses        602      674      +72     

Impacted Files	Coverage Δ
src/CollectionIdentifier.cc	96.15% <ø> (ø)
src/ign.cc	57.40% <0.00%> (-3.47%)	⬇️
src/FuelClient.cc	65.90% <23.33%> (-3.78%)	⬇️
src/Zip.cc	68.75% <50.00%> (-0.77%)	⬇️
src/WorldIter.cc	97.84% <60.00%> (-2.16%)	⬇️
src/Interface.cc	88.46% <100.00%> (ø)
src/LocalCache.cc	79.47% <100.00%> (ø)
src/ModelIdentifier.cc	92.35% <100.00%> (ø)
src/ModelIter.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 5e63428...d2af0da. Read the comment docs.

[adlarkin]

Windows CI, which is required, has a failing test: https://build.osrfoundation.org/job/ign_fuel-tools-pr-win/57/testReport/(root)/HelpersTEST/UriToPathNoAuthority/

[adlarkin]

LGTM, but Windows CI is still failing. Looking at the failure though, it shows that this is not a new failure (age of 21): https://build.osrfoundation.org/job/ign_fuel-tools-pr-win/61/testReport/

So, should Windows CI no longer be a required check? Or do we need to fix this test?

[adlarkin]

Looks like windows CI is still failing with 44c4b23:

C:\Jenkins\workspace\ign_fuel-tools-pr-win\ws\ign-fuel-tools\src\Helpers_TEST.cc:43
Expected equality of these values:
  testStr1
    Which is: "localhost:8000\\\\some\\\\path"
  uriToPath(uri)
    Which is: "localhost:8000\\some\\path"

[mjcarroll]

@adlarkin Windows test should be resolved, but I'm not sure if it's the "correct" answer here.

While I understand that Windows uses backslashes for path separators, I don't believe that URIs should have to, in general. Looking at file URI scheme Wikipedia everything is using forward slashes.

Maybe @nkoenig or @ahcorde has some more insight?

[chapulina]

While I understand that Windows uses backslashes for path separators, I don't believe that URIs should have to, in general.

URIs should always use /, and local paths use each system's separator. So the input to uriToPath is the same for all platforms, but the returned value is platform-specific.

Another thing to keep in mind is that Windows doesn't accept : in paths, so on gazebosim/gz-common#197 common::joinPaths was updated to strip that and other characters. So the expected path on Windows should be localhost8000 instead of localhost:8000.

[mjcarroll]

Another thing to keep in mind is that Windows doesn't accept : in paths, so on gazebosim/gz-common#197 common::joinPaths was updated to strip that and other characters. So the expected path on Windows should be localhost8000 instead of localhost:8000.

That isn't what is currently happening on Windows, though. I just modified the test, and these are the values that make it pass:

uriToPath(common::URI uri{"http://localhost:8000/some/path"}); maps to localhost:8000\some\path
uriToPath(common::URI uri{"http://localhost:8000/some/path/"}); maps to localhost:8000\some\path\

[mjcarroll]

I think the uriToPath should only accept file:// URIs. Everything else should probably fail?

[chapulina]

That isn't what is currently happening on Windows, though.

Yeah I think we haven't fixed uriToPath yet

I think the uriToPath should only accept file:// URIs. Everything else should probably fail?

It looks like it's being used to convert the Fuel Server URL:

https://github.com/ignitionrobotics/ign-fuel-tools/search?q=uritopath

[Blast545]

@chapulina Windows CI is green with the changed tests in 0033e68, do you think this issue with uriToPath should be addressed before merging ign-common#246 or at another time?

👨‍🌾 I think it's ok to remove the tests as long as we add a note there and don't lose track of it. (I can open a follow up issue)

[chapulina]

Windows CI is green with the changed tests in 0033e68

I think we can merge the PR as is for now, but I think we should come back to fix the expectations later, because I think localhost:8000\some\path\ is not a valid Windows path.

do you think this issue with uriToPath should be addressed before merging ign-common#246 or at another time?

I think we don't need to block that for this

[Blast545]

I think we can merge the PR as is for now

Sure, I'll add an issue to fix the expectation at another time. Thanks!

[chapulina]

@mjcarroll , can we clean up the commit history and get this in?

[mjcarroll]

@mjcarroll , can we clean up the commit history and get this in?

Done!