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

Castle Steam #471

Open
wants to merge 137 commits into
base: master
Choose a base branch
from
Open

Castle Steam #471

wants to merge 137 commits into from

Conversation

eugeneloza
Copy link
Member

The primary changes are contained in src/services/steam:

  • castlesteam.pas - An interface for user to interact with. Creates an instance of TCastleSteam when InitSteam is called and keeps it for the duration of the game, automatically freeing everything and logging out of Steam in finalization. Others are internal units.
  • castleinternalsteamapi.pas - function calls to Steam API.
  • castleinternalsteamconstantsandtypes.pas - constants and types translated from Steam API, mostly unused.
  • steamcallback.pas - mostly a copy of Relfos' Steam callback definitions (MIT license), defines memory structures necessary to receive a struct from Steam API.

And a usage example in examples/steam - allows to get/set/clear/indicate achievements.

Note: we may no longer need a523d85

Relfos and others added 30 commits December 16, 2015 12:30
Set app type as console in LPR not in unit
fix version usage in SteamAPI_ISteamClient_GetISteamUserStats
…Achievement, SteamAPI_ISteamUserStats_ClearAchievement
@eugeneloza
Copy link
Member Author

I've renamed the unit and potentially fixed compilation on Delphi 32 bit. Let's see if it works. I didn't see the previous error before you pointed it out - can you ping me where to look for it?

@michaliskambi
Copy link
Member

I didn't see the previous error before you pointed it out - can you ping me where to look for it?

For the errors detected on Delphi (by Jenkinsfile.delphi), you need to look at castle_game_engine_delphi project in Jenkins, https://jenkins.castle-engine.io/job/castle_game_engine_delphi/ .

In section "Pull Requests (4)" ( https://jenkins.castle-engine.io/job/castle_game_engine_delphi/view/change-requests/ ) you can find 2 jobs for every PR, like this:

  1. PR-471-head (testing exactly the code of your PR),
  2. PR-471-merge (testing the PR code automatically merged with master -- in case of merge conflicts, this will just fail).

Admittedly there's no automatic notifications about it, except for admin (me) that gets automatic emails. You have to visit these pages manually, and/or you can also use the Atom feed (linked at the bottom of Jenkins pages, like https://jenkins.castle-engine.io/job/castle_game_engine_delphi/view/change-requests/ ).

For the errors detected on FPC (by Jenkinsfile), look at https://jenkins.castle-engine.io/job/castle_game_engine_organization/job/castle-engine/ . It's similar.

Note: I have just changed the configuration of this on Jenkins. So the above explanation is valid... only since now :) (Previously, by accident, PRs section was not visible, and only branches were visible, but we excluded branches that are duplicated by PRs. It makes more sense now, and it probably doesn't matter how it was before :) )

( For security, it only processes PRs coming from branches in our repository, not forks. )

@eugeneloza
Copy link
Member Author

Interesting... I still see no failures for PR-471-head/PR-471-merge and "last commit" message is unrelated (comes from master, not castle-steam branch).

In castle_game_engine_delphi I see castle-steam crossed out and "This project is currently disabled" as if the branch has been deleted.

What I mean - I still can't see if the build with the last commits has passed or failed.

@eugeneloza
Copy link
Member Author

Oh, I see - it's in the crossed-out castle-steam builds and I see that Delphi 32 build has passed :)

@michaliskambi
Copy link
Member

Interesting... I still see no failures for PR-471-head/PR-471-merge and "last commit" message is unrelated (comes from master, not castle-steam branch).

[...]
Oh, I see - it's in the crossed-out castle-steam builds and I see that Delphi 32 build has passed :)

Short answer: Keep checking in the future are PR-471-head and PR-471-merge valid. Ignore the crossed-out castle-steam state.

Longer answer:

Sorry, that's because I changed Jenkins configuration right before giving you the answer in #471 (comment) . In effect, I created a confusing state for you, by my bad timing :) To explain it better, our current Jenkins configuration (since my post #471 (comment) ) is:

  • We check all the branches from the repository... except branches that are also submitted as a pull request. That is why castle-steam is crossed-out and you should actually not look at it -- it will not be checked when you add more commits there. Once it is submitted as PR, there's no point checking this branch anymore, as PR will check it.

    The reason why we ignore branches submitted also as PRs is just to save Jenkins resources -- it would be wasteful to check this branch, when it is already also checked as a PR.

    But, because of my bad timing, castle-steam contains that old Jenkins failure, and it contains also the fixed version. But you should ignore them at this point :)

  • For pull requests, we create two checks : PR-471-head and PR-471-merge.

    The PR-471-head checks the exact state of the branch. This is what happens if you checkout branch castle-steam and run tests described in Jenkinsfile on them.

    The PR-471-merge checks the state of this branch, if it is merged with master. This is what happens if you checkout branch castle-steam, merge master into it (or maybe the other way around -- check master, merge castle-steam into it, I'm not 100% sure though one could check this), and run tests described in Jenkinsfile on them.

    The last commit on PR-471-merge may indeed look weird -- because it is your branch merged with master, automatically.

    Because of my bad timing, you actually don't see any failure in PR-471-merge and PR-471-head -- because you fixed the failure in parallel to me tweaking Jenkins config. That's OK. Just look at PR-471-merge and PR-471-head in the future and make sure they stay OK.

    Note: Jenkins only scans branches originating from CGE repository -- for security, we don't check forks in Jenkins, so that random people cannot experiment with modifying Jenkinsfile :)

Note: all information above applies equally to FPC and Delphi tests, so https://jenkins.castle-engine.io/job/castle_game_engine_organization/job/castle-engine/ and https://jenkins.castle-engine.io/job/castle_game_engine_delphi/ .

@michaliskambi
Copy link
Member

Here's a screenshot of Jenkins configuration for reference:

j

@eugeneloza
Copy link
Member Author

Oh, I've just noticed that SteamLibraryAvailable is in internal unit. I believe we may want to expose it so that the end-users can also check if steam is not available because it's just still loading, or because the library is not available.

@michaliskambi
Copy link
Member

Oh, I've just noticed that SteamLibraryAvailable is in internal unit. I believe we may want to expose it so that the end-users can also check if steam is not available because it's just still loading, or because the library is not available.

Yes, agreed (make SteamLibraryAvailable available in some non-internal unit).

@eugeneloza
Copy link
Member Author

eugeneloza commented May 10, 2023

PR-471-merge seems to be failing with:

Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.

@michaliskambi
Copy link
Member

PR-471-merge seems to be failing with:

I guess that Windows Jenkins slave was rebooted at a wrong moment. I removed the directory f:\j\workspace\ation_castle-engine_PR-471-merge\ and run the build of it again.

@eugeneloza
Copy link
Member Author

Now it got interrupted externally?

(3104) Compiling ./code/formproject.pas
(3104Sending interrupt signal to process

I've restarted the build again.

@michaliskambi
Copy link
Member

michaliskambi commented May 11, 2023

Now it got interrupted externally?
(3104) Compiling ./code/formproject.pas
(3104Sending interrupt signal to process

This happens when one job was interrupted because another parallel job failed. E.g. if Windows and Linux builds run in parallel, and Jenkins detects an error in Windows job, it immediately interrupts all other jobs (e.g. Linux job, even if it seemed to be OK so far). That's because we specify parallelsAlwaysFailFast() in the Jenkinsfile, and we do it to free executors -- we don't want to keep them busy processing steps, when ultimately the job is doomed to fail already.

So when you see message like above, look for other bug. Look at the boxes table -- and find the first task that "Failed" (red) not just "Aborted" . Instead of boxes (which seem to display now only for last build -- weird) you can also look at tasks as a nested list, https://jenkins.castle-engine.io/job/castle_game_engine_organization/job/castle-engine/view/change-requests/job/PR-471-merge/17/flowGraphTable/ .

In that case, the step that failed is "sh - (5 min 53 sek in self)" within "stage block ((Delphi) Build Examples (Win64)) - (5 min 53 sek in block)". Clicking on "Console Output" of it we get https://jenkins.castle-engine.io/job/castle_game_engine_organization/job/castle-engine/view/change-requests/job/PR-471-merge/17/execution/node/198/log/ with error compiling play_sounds...

And that's my fault. I broke master for a short time, fixed it in 79d158e . During that time, naturally all PR-xxx-merge will fail too. Sorry!

So, there's nothing to do (restarting the job should do the trick). But we know why it failed :)

@eugeneloza
Copy link
Member Author

Ah, and it seems like everything just passed

@eugeneloza
Copy link
Member Author

This happens when one job was interrupted because another parallel job failed

I guess that was another "big" job, because that was the only test that failed for this PR. But yeah, looks like it's fixed now. Got the information for the future :)

…ported OS

Thanks to Peardox for testing on Win32!
@shaunroselt
Copy link

Any update on getting Castle Game Engine onto Steam?

@michaliskambi
Copy link
Member

michaliskambi commented May 15, 2024

Any update on getting Castle Game Engine onto Steam?

This is indeed taking us too long time :) Reviewing and merging this is still high on TODO, and one of the necessary features for engine 7.0 release. I got buried by too many other tasks. So -> I don't yet have an exact time when this will happen, but it will absolutely happen, before 7.0 release, and it's high on my list.

I have just merged latest master to this branch. Testing this PR (on all compilers, platforms) is most welcome :)

@michaliskambi
Copy link
Member

Note: Last run of automatic checks (GitHub Actions) failed at Delphi compilation of Steam example (examples/steam).

But I tested (interactively) now -- hm, it all seems to work, Delphi compilation of examples/steam seems OK, both from Delphi IDE, and from command-line castle-engine compile --compiler=delphi. I will investigate more.

@michaliskambi
Copy link
Member

michaliskambi commented May 15, 2024

Next GitHub Actions run actually detected real problem: Steam units do not compile on 32-bit Raspberry Pi, which is Linux/Arm.

castleinternalsteamcallback.pas(102,17) Error: (7049) Assembler syntax error in operand
/home/ghactions/actions-runner/_work/castle-engine/castle-engine/src/services/steam/castleinternalsteamcallback.pas(102,18) Error: (5004) Unknown identifier "ECX"
/home/ghactions/actions-runner/_work/castle-engine/castle-engine/src/services/steam/castleinternalsteamcallback.pas(116,17) Error: (7049) Assembler syntax error in operand
/home/ghactions/actions-runner/_work/castle-engine/castle-engine/src/services/steam/castleinternalsteamcallback.pas(116,18) Error: (5004) Unknown identifier "ECX"
castleinternalsteamcallback.pas(151) Fatal: (10026) There were 4 errors compiling module, stopping

Linux/Arm is unsupported by Steam most likely. (I never saw Steam on Raspberry Pi, 32 or 64 bit :) It would be cool, but probably not an interesting target (too small) for games, and thus for Steam.) So in principle, we just want to make sure the code compiles and does nothing (doesn't even try to access Steam libs) on this platform.

Optional bonus: Since new macOS and new Windows are both targeting Aarch64 (aka "Arm 64-bit"), Steam most surely supports macOS/Aarch64 and probably supports / will support soon Windows/Aarch64. So this is interesting to investigate and make our code as cross-platform as possible, to support at least Aarch64 if possible, on at least macOS and Windows.

@eugeneloza Do you want to take a look at this? I can provide you access to test (remotely) on both 32-bit Raspberry Pi (Linux on Arm) and 64-bit Raspberry Pi (Linux on Aarch64). Let me know if you're interested :)

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

4 participants