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

Fixes https://github.com/Level/leveldown/issues/196 #235

Merged
merged 1 commit into from
Nov 19, 2015

Conversation

obastemur
Copy link
Contributor

No description provided.

@ralphtheninja
Copy link
Member

+1

@No9 You're the go to guy when it comes to windows :)

@@ -26,9 +26,13 @@
]
, 'conditions': [
['OS == "win"', {
'conditions': [
['MSVS_VERSION != "2015" and MSVS_VERSION != "2013"', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can test it tomorrow but won't this break on 2013 as that version needs the 'port/win' folder?
Also does this now mandate that the --msvs_version has to be passed as part of the build?
If so this will cause issues with down stream libs that depend on this and we should at least have to warn the user that --msvc_version is mandatory on windows somewhere in the output?

@obastemur I also noted here #213 (comment) that there is a patch to go into /deps/leveldb/port-libuv/port_uv.h

Did you come across that?

Thanks for trying to land this in as I couldn't come up we an alternative to the --msvs_version approach when I looked at this back in September so we might just have to take the hit

It also looks like this commit 29282aa from @appelgriebsch is trying to fix the same issue. I seem to remember that the approach in that commit of remming out stdint.h still caused issues on 2015 as the 2015 build defaulted to that file hence the need to ignore it as outlined by @obastemur here.

So I think the steps are

  1. Rollback https://github.com/Level/leveldown/blob/29282aa1de6917da110b116feee4c28f879435a7/deps/leveldb/leveldb-1.18.0/port/win/stdint.h but keep https://github.com/Level/leveldown/blob/29282aa1de6917da110b116feee4c28f879435a7/deps/leveldb/port-libuv/port_uv.h
  2. Confirm the MSVS_VERSION != "2013" test in this patch
  3. Decide if we are going to take the hit of the build option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also does this now mandate that the --msvs_version has to be passed as part of the build?

No. MSVS_VERSION is defined by gyp based on whatever version of VS is selected by gyp etc..

@obastemur I also noted here #213 (comment) that there is a patch to go into /deps/leveldb/port-libuv/port_uv.h
Did you come across that?

Actually I've changed a lot for UWP / VS2015 (see Level/leveldown-mobile) but the original issue is just because of stdint.h so the rest (if there is any) might be subject to another PR.

It also looks like this commit 29282aa from @appelgriebsch is trying to fix the same issue.

IMHO that's not a solution. (shadowing stdint.h)

I've confirmed VS2013. (Don't know VS2012 so didn't add here)

@No9
Copy link
Contributor

No9 commented Nov 18, 2015

OK @obastemur
Agreed "shadowing stdint.h" is not a solution
As it wasn't introduced by this patch I think we should merge this and deal with that as a separate issue.
Sorry for holding up the show but I wanted to be sure.
👍
And thanks again @obastemur for this patch
windows support is very important and your expertise really is appreciated. :)

@No9
Copy link
Contributor

No9 commented Nov 18, 2015

@ralphtheninja do you want to do the honours with the merge
As it's build I don't think we need to push a release for this

@obastemur
Copy link
Contributor Author

@No9 Thanks :)

@ralphtheninja
Copy link
Member

@ralphtheninja do you want to do the honours with the merge
As it's build I don't think we need to push a release for this

Nope! I want you do do it :)

No9 added a commit that referenced this pull request Nov 19, 2015
Fixes #196 and is my first merge into core :)
@No9 No9 merged commit 13e7034 into Level:master Nov 19, 2015
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.

3 participants