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

first try at windows compatibility #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

refack
Copy link

@refack refack commented Apr 4, 2017

WIP for #2

@refack
Copy link
Author

refack commented Apr 4, 2017

So I was stuck a bit with the pipes not closing, until i found out I need to close them on the caller side 🤦
Also I used a C++ smart string for collecting the output. If you don't want C++ that's the only thing need to refactor.
Also I left the options as TODO for today.
(P.S. I didn't see what you do with STDERR)

@refack
Copy link
Author

refack commented Apr 4, 2017

Very sloppy...
Need to add some declations to dukgyp.h

obj/src/dukgyp-test.dukgyp-posix.o: In function `dukgyp_exec_cmd':
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:46: undefined reference to `dukgyp_syscall_throw'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:51: undefined reference to `dukgyp_syscall_throw'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:88: undefined reference to `dukgyp_close_fd'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:89: undefined reference to `dukgyp_close_fd'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:116: undefined reference to `dukgyp_close_fd'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:117: undefined reference to `dukgyp_read_fd'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:118: undefined reference to `dukgyp_close_fd'
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-posix.c:127: undefined reference to `dukgyp_syscall_throw'
obj/src/dukgyp-test.dukgyp-platform.o: In function `dukgyp_native_cp_exec':
/home/travis/build/indutny/dukgyp/out/Release/../../src/dukgyp-platform.c:388: undefined reference to `dukgyp_exec_cmd'

@refack refack force-pushed the windows branch 3 times, most recently from 6001b8e to d0e6d65 Compare April 4, 2017 02:07
@indutny
Copy link
Owner

indutny commented Apr 4, 2017

No C++ allowed 😉 Will take a look tomorrow! Thanks!

@refack refack force-pushed the windows branch 3 times, most recently from 7e9dd2c to c60bb79 Compare April 4, 2017 03:07
@refack
Copy link
Author

refack commented Apr 4, 2017

Removed c++
Merged files with #ifdef

@refack refack changed the title very rough first try at windows dukgyp_exec_cmd first try at windows compatibility Apr 4, 2017
@refack refack force-pushed the windows branch 2 times, most recently from 59a12c8 to 224a36d Compare April 4, 2017 16:47
@refack
Copy link
Author

refack commented Apr 4, 2017

So I'm getting mixes results; when I build with gypkg:

D:\code\3party\dukgyp$ out\Release\dukgyp-test.exe
1..21
ok 1 Platform must be present
not ok 2 Platform must be known
ok 3 Arch must be present
ok 4 Arch must be known

and it crashes ;(

When I build with the VS2017 IDE (I run actual GYP to get vcxproj files)

D:\code\3party\dukgyp$ Default\dukgyp-test.exe        
1..21                                                 
ok 1 Platform must be present                         
not ok 2 Platform must be known                       
ok 3 Arch must be present                             
ok 4 Arch must be known                               
ok 5 cwd() must return string                         
ok 6 argv[0] must be `duktape`                        
ok 7 argv[1] must be proper                           
ok 8 fs.readdirSync() stub must return an array       
not ok 9 fs.realpathSync(random) must throw           
ok 10 fs.realpathSync() should return meaningful data 
ok 11 fs.existsSync() must return true                
ok 12 fs.existsSync() must return false               
ok 13 fs.mkdirpSync() should not throw                
ok 14 fs.writeFileSync() should not throw             
ok 15 fs.readFileSync() should return data            
ok 16 execSync() must return Buffer                   
ok 17 execSync() must return stdout                   
ok 18 execSync() must throw on non-zero exit code     
not ok 19 execSync must respect `cwd` option          
ok 20 binding.env.PATH should not be empty            
ok 21 bindings.env.rnd should be undefined            

(which is pretty good)

@indutny
Copy link
Owner

indutny commented Apr 4, 2017

I guess something is bad with getcwd... Is there any difference in how MSVC links it?

@refack
Copy link
Author

refack commented Apr 4, 2017

That's actually a code-path I did not play with, but I'll compare the build logs

@indutny
Copy link
Owner

indutny commented Apr 4, 2017

Have you tried replacing cwd with just a function that returns string?

@refack
Copy link
Author

refack commented Apr 4, 2017

P.S. Build status

@refack refack force-pushed the windows branch 2 times, most recently from 2385b7f to 8a44edf Compare April 4, 2017 17:11
WIP on indutny#2 (Currently 18/21 tests pass)
- Added AppVeyor for `windows` CI
@refack
Copy link
Author

refack commented Apr 4, 2017

P.P.S. I was skeptical about the whole duktapeing gyp.js, but I'm convinced. A tiny exe that kills GYP 😍

@indutny
Copy link
Owner

indutny commented Apr 4, 2017

Thanks!

@indutny
Copy link
Owner

indutny commented Apr 4, 2017

Would you mind if I'll pick your work from here?

@refack
Copy link
Author

refack commented Apr 4, 2017

Go ahead. If you need something similar (deciphering WIN32 api), ping me.
P.S. I think the crash comes from different memory management of the result of the _strdup in line 44

  • I've setup an appveyor config, you should use it it's super helpfull

@refack
Copy link
Author

refack commented Apr 4, 2017

P.P.S. when working with me, don't be considerate. push/slash/refactor. I'm not afraid a good rebasing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants