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 135 commits into
base: master
Choose a base branch
from
Open

Castle Steam #471

wants to merge 135 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

Another note: I've managed to fix 32-bit compilation in a better way, so we can still support 32-bit platforms (and potentially should support them out-of-the-box, but I don't have somewhere to test those on).

We also can support MAC OSX - but again it wasn't tested.

Activate that support is just uncommenting 3 lines in castleinternalsteamapi with proper library names and adding those libraries.

Also note: as in documentation, we support only SteamWorks version 155. Upgrading the version on the engine-side is trivial, but must be done manually.

@michaliskambi
Copy link
Member

Note 2 Jenkins failures:

  1. Fail to compile with Delphi for Win32:

https://jenkins.castle-engine.io/job/castle_game_engine_delphi/job/castle-steam/

Delphi compiler executing...
Embarcadero Delphi for Win32 compiler version 35.0
Copyright (c) 1983,2021 Embarcadero Technologies, Inc.
f:\j\workspace\_game_engine_delphi_castle-steam\src\transform\castletransform_physics.inc(1267) Warning: W1036 Variable 'RootTransform' might not have been initialized
f:\j\workspace\_game_engine_delphi_castle-steam\src\services\steam\steamcallback.pas(102) Error: E2105 Inline assembler syntax error
f:\j\workspace\_game_engine_delphi_castle-steam\src\services\steam\steamcallback.pas(112) Error: E2105 Inline assembler syntax error
f:\j\workspace\_game_engine_delphi_castle-steam\src\services\steam\castlesteam.pas(601) Fatal: F2063 Could not compile used unit 'SteamCallback.pas'
Exception "Exception":
Failed to compile

Suggested resolution: I'm cool with solving this either way, just as long as Delphi/Win32 applications compile (but possibly ignoring Steam). We don't need Steam on Win32 support, this applies to both Delphi and FPC, new games should use Win64.

  1. macOS "cleanup" fail, but I don't understand why :)

https://jenkins.castle-engine.io/job/castle_game_engine_organization/job/castle-engine/job/castle-steam/

+ repository_cleanup . --remove-unversioned
repository_cleanup: Reverting GIT working copy in: .
Updated 4 paths from the index
repository_cleanup: Cleaning unversioned files.
repository_cleanup: Checking status is empty now.
repository_cleanup: Non-empty status, failed (not cleaned the repo as requested).
 M src/vampyre_imaginglib/README.md
 M src/vampyre_imaginglib/src/Readme.md
 M tests/delphi_tests/README.md
 M tools/build-tool/data/README.md

Note that macOS filesystem is case-sensitive. I didn't expect here any troubles. And I see this PR doesn't touch the mentioned files at all.

Suggested resolution: blame on Jenkins macOS slave hiccup, ignore, unless you have any idea. We'll see next test results.

@michaliskambi
Copy link
Member

steamcallback.pas - mostly a copy of Relfos' Steam callback definitions (MIT license), defines memory structures necessary to receive a struct from Steam API.

Please rename this to castleinternalsteamcallback.pas, so CastleInternal* prefix. Advantage: our PasDoc script automatically excludes such units from API docs, the convention to use CastleInternal* prefix is known to users.

@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!
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

3 participants