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

Make the package go-gettable. #83

Closed
wants to merge 45 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@slimsag
Member

slimsag commented Aug 24, 2014

A lot of discussion took place in #82, lets move it to here now.

@shurcooL can you verify if the update of the GLFW sources (to the revision mentioned in the changelog) slimsag@0b95038 still works on OS X?

I can confirm that building on windows/amd64 and linux/amd64 works A-OK.

I do not have a freebsd machine, and I also do not have a machine with Wayland installed (go get -tags "wayland" github.com/slimsag/glfw3).

Does anyone have comments/concerns/etc regarding this proposal?

Note that this includes @shurcooL's GLFW 3.1 changes.

tapir and others added some commits Nov 26, 2013

Add glfwSetDropCallback() support.
The cgo to Go type conversion in `goDropCB` is unfinished and needs to
be improved.
Merge pull request #68 from go-gl/feature/add-char-mods-callback
Add glfwSetCharModsCallback() support.
Update README.md
changelog updated
Improve documentation style slightly.
Use more idiomatic style for comments, which includes a space after the
//. See
https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Comment_Senten
ces.
Remove references to LockOSThread Wiki example; simply mention
runtime.LockOSThread() in README instead.
Other than docs, there are no code or behavior changes.
Modify README to note runtime.LockOSThread() is needed.
Fix missing runtime.LockOSThread() in example code.
Merge pull request #73 from go-gl/improve-documentation-style
Improve documentation style slightly.
Removed comment
Removed explanation already exists in README
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 24, 2014

Member

Thanks for doing this work and discovery.

Small note, you're missing one commit from devel_glfw3.1 branch, since my fork didn't have it.

Does anyone have comments/concerns/etc regarding this proposal?

Given the big nature of this change, let's give it at least 3+ days before merging. That will give everyone a chance to think it over, provide feedback, etc.

I'll want to learn more about what it will take to update to newer revisions of GLFW.

Would there be any reason to keep the current style wrapper-only available?

Member

dmitshur commented Aug 24, 2014

Thanks for doing this work and discovery.

Small note, you're missing one commit from devel_glfw3.1 branch, since my fork didn't have it.

Does anyone have comments/concerns/etc regarding this proposal?

Given the big nature of this change, let's give it at least 3+ days before merging. That will give everyone a chance to think it over, provide feedback, etc.

I'll want to learn more about what it will take to update to newer revisions of GLFW.

Would there be any reason to keep the current style wrapper-only available?

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 24, 2014

Member

Small note, you're missing one commit from devel_glfw3.1 branch, since my fork didn't have it.

Done.

Given the big nature of this change, let's give it at least 3+ days before merging. That will give everyone a chance to think it over, provide feedback, etc.

Of course, I don't intend to rush anything, just to get the ball rollin' =).

I'll want to learn more about what it will take to update to newer revisions of GLFW.

Would you be able to elaborate on what you mean here?

Would there be any reason to keep the current style wrapper-only available?

Here are reasons that come to mind for me:

  • CMAKE options that are not yet added by us as build tags, if someone finds them really important.
  • If you intend to use some other CGO library that uses GLFW3 as well, then you might want to dynamically link against it to avoid symbol collisions (but it would really be a very strange situation and I don't know of many game development libraries that use GLFW3 on their own).
Member

slimsag commented Aug 24, 2014

Small note, you're missing one commit from devel_glfw3.1 branch, since my fork didn't have it.

Done.

Given the big nature of this change, let's give it at least 3+ days before merging. That will give everyone a chance to think it over, provide feedback, etc.

Of course, I don't intend to rush anything, just to get the ball rollin' =).

I'll want to learn more about what it will take to update to newer revisions of GLFW.

Would you be able to elaborate on what you mean here?

Would there be any reason to keep the current style wrapper-only available?

Here are reasons that come to mind for me:

  • CMAKE options that are not yet added by us as build tags, if someone finds them really important.
  • If you intend to use some other CGO library that uses GLFW3 as well, then you might want to dynamically link against it to avoid symbol collisions (but it would really be a very strange situation and I don't know of many game development libraries that use GLFW3 on their own).
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 24, 2014

Member

I'll want to learn more about what it will take to update to newer revisions of GLFW.

Currently, the added GLFW source is revision 5d525c4a5f9da0b8744f29affbf77d3a9580905c, which at this time is the latest. Suppose a week later new commits are pushed and I'd like to add that to a glfw3 with 5d525c4a5f9da0b8744f29affbf77d3a9580905c. How would I do that.

That's what I want to try/learn more about. Perhaps make a Makefile or bash script, etc.

Member

dmitshur commented Aug 24, 2014

I'll want to learn more about what it will take to update to newer revisions of GLFW.

Currently, the added GLFW source is revision 5d525c4a5f9da0b8744f29affbf77d3a9580905c, which at this time is the latest. Suppose a week later new commits are pushed and I'd like to add that to a glfw3 with 5d525c4a5f9da0b8744f29affbf77d3a9580905c. How would I do that.

That's what I want to try/learn more about. Perhaps make a Makefile or bash script, etc.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 24, 2014

Member

I see, that makes sense. This seems to work for me:

# Precursor:
cd $GOPATH/src/github.com/go-gl/glfw3

# Actual script:
cd glfw/
git init
git add -A .
git remote add origin https://github.com/glfw/glfw
git pull origin master

# Now the glfw/ folder is at the latest revision, make our update commit.
cd ..
git add glfw/
git commit -m "Updated GLFW sources."
git push

I've tested it with actual changes to GLFW's sources in my own repository and it works good. I don't care about the bash script or not -- but if you want I can certainly add one to do the above (or something else?).

Member

slimsag commented Aug 24, 2014

I see, that makes sense. This seems to work for me:

# Precursor:
cd $GOPATH/src/github.com/go-gl/glfw3

# Actual script:
cd glfw/
git init
git add -A .
git remote add origin https://github.com/glfw/glfw
git pull origin master

# Now the glfw/ folder is at the latest revision, make our update commit.
cd ..
git add glfw/
git commit -m "Updated GLFW sources."
git push

I've tested it with actual changes to GLFW's sources in my own repository and it works good. I don't care about the bash script or not -- but if you want I can certainly add one to do the above (or something else?).

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 24, 2014

Member

That's great, thanks. No need for a script at this time.

Member

dmitshur commented Aug 24, 2014

That's great, thanks. No need for a script at this time.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 25, 2014

Member

I still need to test this. Rather large change and I'm not sure about having to maintain our own GLFW source.

Just a quick look at the code: Why not merge all darwin.go files?
Also the glfw
.c files? I don't see a reason they should be separated.

EDIT: Why do we have 3.1 changes on this PR?

Member

tapir commented Aug 25, 2014

I still need to test this. Rather large change and I'm not sure about having to maintain our own GLFW source.

Just a quick look at the code: Why not merge all darwin.go files?
Also the glfw
.c files? I don't see a reason they should be separated.

EDIT: Why do we have 3.1 changes on this PR?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 25, 2014

Member

EDIT: Why do we have 3.1 changes on this PR?

IMO the advantages of using the current 3.1 with many improvements, bug fixes, but a few remaining known issues outweigh using the older 3.0.4 that does not have many of 3.1's improvements and bug fixes.

However, I understand the desire to use the latest GLFW may not be acceptable for everyone's needs.

What about following Chrome's idea of having channels. We could have glfw3 stable, which follows official GLFW releases, and something like glfw3-dev that is updated more often.

Member

dmitshur commented Aug 25, 2014

EDIT: Why do we have 3.1 changes on this PR?

IMO the advantages of using the current 3.1 with many improvements, bug fixes, but a few remaining known issues outweigh using the older 3.0.4 that does not have many of 3.1's improvements and bug fixes.

However, I understand the desire to use the latest GLFW may not be acceptable for everyone's needs.

What about following Chrome's idea of having channels. We could have glfw3 stable, which follows official GLFW releases, and something like glfw3-dev that is updated more often.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 25, 2014

Member

It's better to separate API breaking changes like in 3.1 and this. This is a change that can be useful for both master and devel. In the end we will tag master as "3.0" so that people can continue using it. If this change is useful it should exist in "3.0" also that's why I think the better method is to create a separate PR for master and then rebase devel.

Most definitely we should not use an unstable GFLW version for our stable go package.
Devel branch can have the latest git version of GLFW but the master should only have the latest stable source of GLFW.

Member

tapir commented Aug 25, 2014

It's better to separate API breaking changes like in 3.1 and this. This is a change that can be useful for both master and devel. In the end we will tag master as "3.0" so that people can continue using it. If this change is useful it should exist in "3.0" also that's why I think the better method is to create a separate PR for master and then rebase devel.

Most definitely we should not use an unstable GFLW version for our stable go package.
Devel branch can have the latest git version of GLFW but the master should only have the latest stable source of GLFW.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 25, 2014

Member

Branches are not go-gettable, that's why my proposal is to have a distinct repo with that devel branch.

Member

dmitshur commented Aug 25, 2014

Branches are not go-gettable, that's why my proposal is to have a distinct repo with that devel branch.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 25, 2014

Member

How many people need to "go get" devel branch? Most people should use stable anyway.
Having 2 separate repos seems counter-intuitive to me with very little advantage.

Member

tapir commented Aug 25, 2014

How many people need to "go get" devel branch? Most people should use stable anyway.
Having 2 separate repos seems counter-intuitive to me with very little advantage.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 25, 2014

Member

Just a quick look at the code: Why not merge all darwin.go files?
Also the glfw
.c files? I don't see a reason they should be separated.

C files can have multiple functions declared with the same exact name. C has no concept of namespaces and these functions would collide and cause the C compiler to give you an error. For this reason people declare static functions throughout their code -- static functions can have the same name but they must exist in separate object files or else they will collide. Most C programmers (including GLFW's source throughout) use this practice and this is why they need to be in separate files.

EDIT: Why do we have 3.1 changes on this PR?

@shurcooL made the 3.1 changes already and I didn't want to write this against old code that would be gone in a short time. I understand the desire to wait to merge this until GLFW 3.1 is officially released though, and we can certainly do that.

I don't really have a preference as to whether the branches are go-gettable or not. The stable branch gets API-incompatible changes whenever a new GLFW version is released anyway -- so people will probably just go the vendoring route anyway IMHO.

Member

slimsag commented Aug 25, 2014

Just a quick look at the code: Why not merge all darwin.go files?
Also the glfw
.c files? I don't see a reason they should be separated.

C files can have multiple functions declared with the same exact name. C has no concept of namespaces and these functions would collide and cause the C compiler to give you an error. For this reason people declare static functions throughout their code -- static functions can have the same name but they must exist in separate object files or else they will collide. Most C programmers (including GLFW's source throughout) use this practice and this is why they need to be in separate files.

EDIT: Why do we have 3.1 changes on this PR?

@shurcooL made the 3.1 changes already and I didn't want to write this against old code that would be gone in a short time. I understand the desire to wait to merge this until GLFW 3.1 is officially released though, and we can certainly do that.

I don't really have a preference as to whether the branches are go-gettable or not. The stable branch gets API-incompatible changes whenever a new GLFW version is released anyway -- so people will probably just go the vendoring route anyway IMHO.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 25, 2014

Member
// glfw.c
#if defined(_GLFW_X11) || defined(_GLFW_WAYLAND)
    #include "glfw/src/xkb_unicode.c"
    #include "glfw/src/linux_joystick.c"
    #include "glfw/src/posix_time.c"
#endif

#if defined(_GLFW_COCOA) || defined(_GLFW_X11) || defined(_GLFW_WAYLAND)
    #include "glfw/src/posix_tls.c"
#endif

#ifdef _GLFW_EGL
    #include "glfw/src/egl_context.c"
#endif

#ifdef _GLFW_GLX
    #include "glfw/src/glx_context.c"
#endif

#ifdef _GLFW_COCOA
    #include "glfw/src/mach_time.c"
#endif

#ifdef _GLFW_WGL
    #include "glfw/src/wgl_context.c"
#endif

#ifdef _GLFW_WIN32
    #include "glfw/src/win32_clipboard.c"
    #include "glfw/src/win32_init.c"
    #include "glfw/src/win32_monitor.c"
    #include "glfw/src/win32_time.c"
    #include "glfw/src/win32_tls.c"
    #include "glfw/src/win32_window.c"
    #include "glfw/src/winmm_joystick.c"
#endif

#ifdef _GLFW_WAYLAND
    #include "glfw/src/wl_clipboard.c"
    #include "glfw/src/wl_init.c"
    #include "glfw/src/wl_monitor.c"
    #include "glfw/src/wl_window.c"
#endif

#ifdef _GLFW_X11
    #include "glfw/src/x11_clipboard.c"
    #include "glfw/src/x11_init.c"
    #include "glfw/src/x11_monitor.c"
    #include "glfw/src/x11_window.c"
#endif

#include "glfw/src/clipboard.c"
#include "glfw/src/context.c"
#include "glfw/src/init.c"
#include "glfw/src/input.c"
#include "glfw/src/joystick.c"
#include "glfw/src/monitor.c"
#include "glfw/src/time.c"
#include "glfw/src/window.c"

I was not talking about the actual GLFW source but the shim files that start with "glfw_".
Tried above on mingw32, and it builds without a problem.

Member

tapir commented Aug 25, 2014

// glfw.c
#if defined(_GLFW_X11) || defined(_GLFW_WAYLAND)
    #include "glfw/src/xkb_unicode.c"
    #include "glfw/src/linux_joystick.c"
    #include "glfw/src/posix_time.c"
#endif

#if defined(_GLFW_COCOA) || defined(_GLFW_X11) || defined(_GLFW_WAYLAND)
    #include "glfw/src/posix_tls.c"
#endif

#ifdef _GLFW_EGL
    #include "glfw/src/egl_context.c"
#endif

#ifdef _GLFW_GLX
    #include "glfw/src/glx_context.c"
#endif

#ifdef _GLFW_COCOA
    #include "glfw/src/mach_time.c"
#endif

#ifdef _GLFW_WGL
    #include "glfw/src/wgl_context.c"
#endif

#ifdef _GLFW_WIN32
    #include "glfw/src/win32_clipboard.c"
    #include "glfw/src/win32_init.c"
    #include "glfw/src/win32_monitor.c"
    #include "glfw/src/win32_time.c"
    #include "glfw/src/win32_tls.c"
    #include "glfw/src/win32_window.c"
    #include "glfw/src/winmm_joystick.c"
#endif

#ifdef _GLFW_WAYLAND
    #include "glfw/src/wl_clipboard.c"
    #include "glfw/src/wl_init.c"
    #include "glfw/src/wl_monitor.c"
    #include "glfw/src/wl_window.c"
#endif

#ifdef _GLFW_X11
    #include "glfw/src/x11_clipboard.c"
    #include "glfw/src/x11_init.c"
    #include "glfw/src/x11_monitor.c"
    #include "glfw/src/x11_window.c"
#endif

#include "glfw/src/clipboard.c"
#include "glfw/src/context.c"
#include "glfw/src/init.c"
#include "glfw/src/input.c"
#include "glfw/src/joystick.c"
#include "glfw/src/monitor.c"
#include "glfw/src/time.c"
#include "glfw/src/window.c"

I was not talking about the actual GLFW source but the shim files that start with "glfw_".
Tried above on mingw32, and it builds without a problem.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 25, 2014

Member

Also this would be more organized as well:

// cocoa_darwin.go
package glfw3

/*
#cgo CFLAGS: -x objective-c
#ifdef _GLFW_COCOA
    #include "glfw/src/cocoa_clipboard.m"
    #include "glfw/src/cocoa_init.m"
    #include "glfw/src/cocoa_monitor.m"
    #include "glfw/src/cocoa_window.m"
    #include "glfw/src/iokit_joystick.m"
    #include "glfw/src/nsgl_context.m"
#endif
*/
import "C"
Member

tapir commented Aug 25, 2014

Also this would be more organized as well:

// cocoa_darwin.go
package glfw3

/*
#cgo CFLAGS: -x objective-c
#ifdef _GLFW_COCOA
    #include "glfw/src/cocoa_clipboard.m"
    #include "glfw/src/cocoa_init.m"
    #include "glfw/src/cocoa_monitor.m"
    #include "glfw/src/cocoa_window.m"
    #include "glfw/src/iokit_joystick.m"
    #include "glfw/src/nsgl_context.m"
#endif
*/
import "C"
@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 25, 2014

Member

I was not talking about the actual GLFW source but the shim files that start with "glfw_".
Tried above on mingw32, and it builds without a problem.

I am not talking about the sources. I am talking about the many glfw_foo_bar.c shim files in the root directory. The C code you pasted above produces the following errors on Linux:

In file included from ./build.c:48:0:
./glfw/src/x11_window.c:89:12: error: redefinition of ‘translateKey’
 static int translateKey(int keycode)
            ^
In file included from ./build.c:46:0:
./glfw/src/x11_init.c:40:12: note: previous definition of ‘translateKey’ was here
 static int translateKey(int keyCode)
            ^

This is because on Linux there are two statically declared translateKey functions, one in x11_window.c and the other in x11_init.c as shown above. This isn't really negotiable at all -- yes your single-file version above works on Windows and OS X, but only right now. If the GLFW developers add any other static functions with the same names on those platforms it will break.

Member

slimsag commented Aug 25, 2014

I was not talking about the actual GLFW source but the shim files that start with "glfw_".
Tried above on mingw32, and it builds without a problem.

I am not talking about the sources. I am talking about the many glfw_foo_bar.c shim files in the root directory. The C code you pasted above produces the following errors on Linux:

In file included from ./build.c:48:0:
./glfw/src/x11_window.c:89:12: error: redefinition of ‘translateKey’
 static int translateKey(int keycode)
            ^
In file included from ./build.c:46:0:
./glfw/src/x11_init.c:40:12: note: previous definition of ‘translateKey’ was here
 static int translateKey(int keyCode)
            ^

This is because on Linux there are two statically declared translateKey functions, one in x11_window.c and the other in x11_init.c as shown above. This isn't really negotiable at all -- yes your single-file version above works on Windows and OS X, but only right now. If the GLFW developers add any other static functions with the same names on those platforms it will break.

@tapir

This comment has been minimized.

Show comment
Hide comment
@tapir

tapir Aug 25, 2014

Member

I see.

Any reason to separate cocoa*_darwin.go files though?

I'm OK with merging this change as long as we keep the master branch in sync with stable GLFW. So I would really have these changes commit against master with GLFW 3.0.4 source included in the tree (glfw/glfw@eab31f2)

Then we can have devel rebased to master and update the GLFW source to latest revision in git.

Member

tapir commented Aug 25, 2014

I see.

Any reason to separate cocoa*_darwin.go files though?

I'm OK with merging this change as long as we keep the master branch in sync with stable GLFW. So I would really have these changes commit against master with GLFW 3.0.4 source included in the tree (glfw/glfw@eab31f2)

Then we can have devel rebased to master and update the GLFW source to latest revision in git.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 25, 2014

Member

Any reason to separate cocoa*_darwin.go files though?

I don't know enough about Objective-C to know for certain -- but just looking at this Apple page makes me think it may be a near-identical situation. Aside from that, I only argue for consistency reasons.

Member

slimsag commented Aug 25, 2014

Any reason to separate cocoa*_darwin.go files though?

I don't know enough about Objective-C to know for certain -- but just looking at this Apple page makes me think it may be a near-identical situation. Aside from that, I only argue for consistency reasons.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 26, 2014

Member

Having 2 separate repos seems counter-intuitive to me with very little advantage.

Yeah, I agree. I think go-gl can host the stable version and one of our personal github accounts can host a devel version.

But I'm okay with waiting for 3.1 to merge this PR into go-gl/glfw3. It can be merged into the 3.1 devel branch before then.

Member

dmitshur commented Aug 26, 2014

Having 2 separate repos seems counter-intuitive to me with very little advantage.

Yeah, I agree. I think go-gl can host the stable version and one of our personal github accounts can host a devel version.

But I'm okay with waiting for 3.1 to merge this PR into go-gl/glfw3. It can be merged into the 3.1 devel branch before then.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 28, 2014

Member

I've made a new PR to merge this into the 3.1 development branch, please see #84.

Member

slimsag commented Aug 28, 2014

I've made a new PR to merge this into the 3.1 development branch, please see #84.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment