Windows: expand path names on command line #5044
Conversation
|
AFAICT the MacOS Python failure looks unrelated. |
|
Is linking setargv.obj not working any more? |
|
@xfxyjwf : I thought it never worked so users needed workarounds like this one: #3957 (comment) Did it actually work in newer releases? |
|
(FYI, I can't work in this again until 2018-08-27.) |
|
My understanding is that linking setargv.obj should work, but the protoc binary we release for windows is not built with bazel so it doesn't include that change. I'll test it a bit on windows and see if changing our protoc release script to also link setargv.obj can fix the issues. |
|
Thanks @xfxyjwf , I see. |
|
@laszlocsomor Do you know what the current status of this pull request is? Feng left to join a startup, so I can take over reviewing this. I don't really have much context on the Windows path name issue here yet, though. |
|
@acozzette : Thanks for pinging! This PR was never merged. Protoc 3.6.1 still does not support wildcards on Windows, so I guess the current solution (linking the Microsoft-shipped |
There was a problem hiding this comment.
@laszlocsomor Thanks so much for this PR and sorry for taking so long to get back to you. I made just a few comments.
| @@ -355,6 +360,92 @@ wstring testonly_utf8_to_winpath(const char* path) { | |||
| return as_windows_path(path, &wpath) ? wpath : wstring(); | |||
| } | |||
|
|
|||
| bool expand_wildcards( | |||
There was a problem hiding this comment.
I think based on our naming style this should be ExpandWildcards instead of expand_wildcards.
| return ExpandWildcardsResult::kSuccess; | ||
| } | ||
|
|
||
| #ifdef SUPPORT_LONGPATHS |
There was a problem hiding this comment.
Is there a way to unify some of this logic between the SUPPORT_LONGPATHS and non-SUPPORT_LONGPATHS cases? It seems like the logic is mostly the same, so I wonder if templates would make it possible to unify the two cases.
There was a problem hiding this comment.
I tried, it makes the code messier.
src/google/protobuf/stubs/io_win32.h
Outdated
| // and it matched at least one file. | ||
| // The function returns false if the path contained wildcards but it did not | ||
| // match any files. | ||
| LIBPROTOBUF_EXPORT bool expand_wildcards( |
There was a problem hiding this comment.
Shouldn't this return an ExpandWildcardsResult instead of bool?
|
@acozzette : Thanks for the review! And sorry about the delay. Lots of other things to do. I'll get back to this thread as soon as I can. |
|
@laszlocsomor @acozzette any update on the PR? |
|
Can't trigger presubmit tests, "release notes: yes|no" label is missing. |
|
@xycui : Sorry for the long silence. |
|
@acozzette : Could you add "release notes: no" tag please? |
|
Thanks, @laszlocsomor! |
|
Thanks for following through! |
We can now switch back to a current version of the protocol since the PR protocolbuffers/protobuf#5044 has been merged to master a released.
* Upgrade protobuf to v3.9.1 for AppVeyor We can now switch back to a current version of the protocol since the PR protocolbuffers/protobuf#5044 has been merged to master a released. * Bump drone.io Docker image from 1.0 to 1.1 (#329)
release notes: no
Fixes #3957