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

lib: fixing slash to HOST_DIRSEP #3856

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

neteler
Copy link
Member

@neteler neteler commented Jun 16, 2024

Changes hardcoded slash (/) in path to HOST_DIRSEP for portability.

Fixes #3296

Fixes hardcoded slash to HOST_DIRSEP.

Fixes OSGeo#3296
@neteler neteler added the C Related code is in C label Jun 16, 2024
@neteler neteler added this to the 8.5.0 milestone Jun 16, 2024
@neteler neteler requested a review from hellik June 16, 2024 22:36
Copy link
Member

@hellik hellik left a comment

Choose a reason for hiding this comment

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

I can't test right now, but looks good

@echoix
Copy link
Member

echoix commented Jun 17, 2024

Failures are weird, and Windows has way more, many tests exited right away, as instead of taking 50min, tests took 11 min.

@nilason
Copy link
Contributor

nilason commented Jun 17, 2024

This ends up with mixed / and \ which I believe was part of the original problem:
ERROR: Unable to make mapset element .tmp\unknown (C:\Users\runneradmin\nc_spm_full_v2alpha2\__general_g_remove_test_g_remove_fv_az587_258_3624/.tmp\unknown.
This particular forward slash originates from

int make_mapset_element_impl(const char *p_path, const char *p_element,

@github-actions github-actions bot added raster Related to raster data processing database Related to database management labels Jun 17, 2024
@neteler
Copy link
Member Author

neteler commented Jun 17, 2024

In 6749496 I have changed more occurrences of '/' to HOST_DIRSEP. Hope I got it right, pls review.

The search I used was:

ag " '/'"

@neteler neteler changed the title lib/gis: lib/display, fixing slash to HOST_DIRSEP lib: fixing slash to HOST_DIRSEP Jun 17, 2024
@nilason
Copy link
Contributor

nilason commented Jun 17, 2024

Let us see the results of CI runners first.

There are a number of hardcoded cases with / for files in $GISBASE, e.g. etc/symbols, etc/proj etc. Perhaps it is not necessary to change every case now (locating $GISBASE with contents will have to have a somewhat different solution for FHS anyway).

@neteler
Copy link
Member Author

neteler commented Jun 17, 2024

Failures like this in the log, still mixed...:

ERROR: Unable to make mapset element vector/random_points (C:\Users\runneradmin\nc_spm_full_v2alpha2\__vector_v_what_rast3_test.v.what.rast3_fv_az1783_710_4036\vector/random_points): No such file or directory

@wenzeslaus
Copy link
Member

I'm confused about the discussion here.

Changes hardcoded slash (/) in path to HOST_DIRSEP for portability.

Portability is not an issue. / is a perfectly fine directory separator on Windows (AltDirectorySeparatorChar).

This ends up with mixed / and \ which I believe was part of the original problem:
ERROR: Unable to make mapset element...

Are you saying that the mixing causes the error, forward slash, or are we simply trying to fix the inconsistency in the error message?

@hellik
Copy link
Member

hellik commented Jun 19, 2024

Mixing may not be good, especially when scripting comes into play.

@echoix
Copy link
Member

echoix commented Jun 19, 2024

Mixing may not be good, especially when scripting comes into play.

We'd need to verify by manually doing a single fix and seeing if this one is fixed, but I think that it is only problematic from a Cygwin/msys2 point of there are both. But we need to see that the changes here remove some errors.

@nilason
Copy link
Contributor

nilason commented Jun 19, 2024

Portability is not an issue. / is a perfectly fine directory separator on Windows (AltDirectorySeparatorChar).

That is unfortunately only valid for .NET API, perhaps https://superuser.com/a/176395 may throw some light into the issue at hand. I'm afraid that our code is full of hardcoded /: changing all that will be a huge effort, perhaps unnecessarily.

I'm not aware of current other issues than #3296 that reports this kind of problem, so it maybe a less invasive/ more immediate fix for g.tempfile to convert the tempfile path with G_convert_dirseps_to_host().

This problem with mixed use of characters originates from the fact that libgis make an effort to use HOST_DIRSEP, but pretty much everywhere else the hardcoded / is used.

@echoix
Copy link
Member

echoix commented Jun 19, 2024

Apart from the cygpath tool, what other conversions does msys2 do? There's this docs https://www.msys2.org/docs/filesystem-paths/

They say that they convert automatically env vars, process arguments. Native tools would work with both if they don't process the path too much and forwards it the the system API, which is what Cygwin's POSIX API does (forwarding to the Windows API)

*p = 0;
}

/* now append element, one directory at a time, to path */
while (1) {
if (*element == '/' || *element == 0) {
if (*element == HOST_DIRSEP || *element == 0) {
*p = 0;
char *msg = NULL;

Copy link
Member Author

Choose a reason for hiding this comment

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

Somewhere in the lines below one more change is needed to avoid issues like this (see OSGeo4W CI run log):

+ v.in.ascii --o -z format=standard input=data/random_points.ref output=random_points
ERROR: Unable to make mapset element vector/random_points
 (C:\Users\runneradmin\nc_spm_full_v2alpha2\__vector_v_what_rast3_test.v.what.rast3_fv_az845_621_4592\vector/random_points): No such file or directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C database Related to database management display libraries raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Unix to Windows Portability bug in g.tempfile
5 participants