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.

Sign up for GitHub

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

Jump to bottom

Remove duplicate exe name with zig run on Windows #2665

Merged
merged 1 commit into from Jun 17, 2019
Merged

Remove duplicate exe name with zig run on Windows #2665

merged 1 commit into from Jun 17, 2019

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented Jun 13, 2019 •
edited

Fix issue #2686

shawnl
shawnl approved these changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@shawnl shawnl left a comment

Choose a reason for hiding this comment

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

Considering https://github.com/ziglang/zig/blob/06e7533b29b596a55de75735e5fa8c0aaf94b9b8/std/child_process.zig#L628 which comes from https://github.com/ziglang/zig/blob/06e7533b29b596a55de75735e5fa8c0aaf94b9b8/std/child_process.zig#L524

and extracts self.argv[0]

and looking at https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-createprocessa

LGTM

@shawnl
Copy link
Contributor

shawnl commented Jun 13, 2019

Why did you force push? You didn't change anything.

@marler8997
Copy link
Contributor Author

rebase

@marler8997
Copy link
Contributor Author

Seeing this same error in multiple pull requests:

/Users/vsts/agent/2.152.1/work/1/s/std/hash_map.zig:407:21: error: expression value is ignored
/Users/vsts/agent/2.152.1/work/1/s/std/hash_map.zig:447:27: error: expression value is ignored
/Users/vsts/agent/2.152.1/work/1/s/std/hash_map.zig:448:27: error: expression value is ignored
/Users/vsts/agent/2.152.1/work/1/s/std/hash_map.zig:449:27: error: expression value is ignored

@shawnl
Copy link
Contributor

shawnl commented Jun 13, 2019 •
edited

That was most likely caused by the most recent commit 80fa871

@marler8997
Copy link
Contributor Author

Thanks for the info. Hopefully will get fixed soon?

@daurnimator
Copy link
Collaborator

That was caused by the most recent commit (which presumably did not go through a pull request or this would have been spotted) 80fa871

#2664

@marler8997
Copy link
Contributor Author

Fix here: #2666

@marler8997 marler8997 force-pushed the windowsCommandLineFix branch 2 times, most recently from acec409 to 5bba981 Compare June 14, 2019 08:52
@marler8997 marler8997 changed the title Remove application name from windows command-line Remove duplicate exe name with zig run Jun 14, 2019
@marler8997
Copy link
Contributor Author

marler8997 commented Jun 14, 2019 •
edited

I was mistaken. It looks like CreateProcess does expect the executable to be apart of the lpCommandLine parameter. It turns out the error was actually just a bug in src\main.cpp and/or src\os.cpp. It was adding the executable name to the argument list twice.

I've updated the PR to fix this problem in main.cpp/os.cpp.

@marler8997 marler8997 changed the title Remove duplicate exe name with zig run Remove duplicate exe name with zig run on Windows Jun 14, 2019
@marler8997 marler8997 force-pushed the windowsCommandLineFix branch 4 times, most recently from 95a1d98 to f69568c Compare June 14, 2019 20:40
@marler8997 marler8997 mentioned this pull request Jun 16, 2019
Closed
andrewrk
andrewrk reviewed Jun 16, 2019
View reviewed changes
src/main.cpp Outdated
@@ -1160,7 +1160,8 @@ int main(int argc, char **argv) {

args.pop();
Termination term;
os_spawn_process(exec_path, args, &term);
// use NULL for exe, it's already included in args
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making the API in os.cpp not even have the exe parameter? It would require the first arg to be the exe. That's what we ended up doing in the zig standard library and I think it's been a reasonable abstraction. That's how I was planning to fix this issue in the stage1 compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. I'll see if I can update the PR tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR, let me know if you'd like any more changes.

@marler8997 marler8997 force-pushed the windowsCommandLineFix branch 5 times, most recently from 6f4db25 to 2977c3e Compare June 17, 2019 03:58
andrewrk
andrewrk approved these changes Jun 17, 2019
View reviewed changes
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the new API thing

@andrewrk andrewrk merged commit 21dff1c into ziglang:master Jun 17, 2019
@marler8997 marler8997 deleted the windowsCommandLineFix branch June 17, 2019 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewrk andrewrk andrewrk approved these changes

@shawnl shawnl shawnl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marler8997 @shawnl @daurnimator @andrewrk