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

bump janet version to 1.31.0 #2306

Merged
merged 2 commits into from
Sep 23, 2023
Merged

bump janet version to 1.31.0 #2306

merged 2 commits into from
Sep 23, 2023

Conversation

AlecTroemel
Copy link
Sponsor Contributor

@sogaiu
Copy link

sogaiu commented Sep 18, 2023

Mmm, more build-related fun...

Ok, I'm going through the logs and I see ultimately that:

state.obj : error LNK2019: unresolved external symbol _InterlockedIncrement referenced in function _janet_interpreter_interrupt [D:\a\TIC-80\TIC-80\build\janet.vcxproj]
state.obj : error LNK2019: unresolved external symbol _InterlockedDecrement referenced in function _janet_interpreter_interrupt_handled [D:\a\TIC-80\TIC-80\build\janet.vcxproj]
build\janet_boot.exe : fatal error LNK1120: 2 unresolved externals [D:\a\TIC-80\TIC-80\build\janet.vcxproj]

via: https://github.com/nesbox/TIC-80/actions/runs/6223953191/job/16891060122?pr=2306#step:4:1692

Looking back earlier, I see:

src\core\state.c(62): warning C4013: 'InterlockedIncrement' undefined; assuming extern returning int [D:\a\TIC-80\TIC-80\build\janet.vcxproj]
src\core\state.c(71): warning C4013: 'InterlockedDecrement' undefined; assuming extern returning int [D:\a\TIC-80\TIC-80\build\janet.vcxproj]

via: https://github.com/nesbox/TIC-80/actions/runs/6223953191/job/16891060122?pr=2306#step:4:1535

All of the Windows builds seem to be affected in a similar manner -- I believe searching for "Interlocked" shows this.

Found some info about "warning C4013".

Here is some InterlockedIncrement info.

On first glance, this looks related to: janet-lang/janet@ffd79c6

Perhaps if we're lucky it might be a matter of just adding some header file info to some Janet source file like state.c or state.h...


On a side note, I don't know how practical it is, but if we can somehow test changes that happen to Janet sooner with TIC-80, the context of the changes might be more fully in bakpakin's mind (and/or other folks' minds) so it might be easier to discuss TIC-80 build issues. Also, the sooner the detection, the less other folks will have had a chance to start relying on changes...

@sogaiu
Copy link

sogaiu commented Sep 19, 2023

Please ignore the following -- am investigating further. Leaving it for having a record.

@AlecTroemel Ok, I tried changing `state.c` a bit and attempted a local Windows build. That seems to have worked.

The change I made was:

diff --git a/src/core/state.c b/src/core/state.c
index 4c976da..5f34b07 100644
--- a/src/core/state.c
+++ b/src/core/state.c
@@ -27,6 +27,10 @@
 #include "util.h"
 #endif
 
+#ifdef JANET_WINDOWS
+#include "windows.h"
+#endif
+
 JANET_THREAD_LOCAL JanetVM janet_vm;
 
 JanetVM *janet_local_vm(void) {

I don't know if that's really an appropriate change (e.g. may be winnt.h would be better or even if there should be a change [1]), but at least I get a working tic80.exe.

When you get a chance, would you mind updating the PR to see if the CI stuff works?


Note that build/janet/janetconf.h's values seem off -- some of these. So may be it makes sense to tweak?


[1] I tried building janet 1.31.0 on Windows (outside of the context of TIC-80 and thus without the changes to state.c mentioned above) and it builds fine so I'm not sure what's going on here.

@sogaiu
Copy link

sogaiu commented Sep 19, 2023

@AlecTroemel build/janet/janetconf.h's values seem off -- specifically some of these. See below for specifics.

Also, Janet's janetconf.h in the 1.31.0 release had slightly off values too (a PR for that was merged), so I think it might make sense to update to the commit that has those changes.

In summary, I believe I was able to build under Windows locally [1] with at least the following changes:

  • Update build/janet/janetconf.h's values (see the right side of this for what I tried)
  • Update vendor/janet to this commit of Janet

I'll give this another try from scratch and report back.

Update: Unfortunately, it doesn't appear to be enough. It's odd because building Janet on its own in Windows is working here. Perhaps there is something different about invoking build_win.bat via the cmake-generated things...

The following patch to Janet does seem to make things work though...

diff --git a/src/core/state.c b/src/core/state.c
index 4c976da..b17fb9d 100644
--- a/src/core/state.c
+++ b/src/core/state.c
@@ -53,6 +53,10 @@ void janet_vm_load(JanetVM *from) {
     janet_vm = *from;
 }
 
+#ifdef JANET_WINDOWS
+#include "windows.h"
+#endif
+
 /* Trigger suspension of the Janet vm by trying to
  * exit the interpeter loop when convenient. You can optionally
  * use NULL to interrupt the current VM when convenient */

[1] It was nice to have these notes. The footnote in that comment was handy too ;)

@AlecTroemel
Copy link
Sponsor Contributor Author

@sogaiu i've updated the janetconf.h. I've also updated janet to this commit, since it included the windows fix you mentioned in this issue

@sogaiu
Copy link

sogaiu commented Sep 19, 2023

Thanks!

@nesbox nesbox merged commit 7b86c1c into nesbox:main Sep 23, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants