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.

Sign up for GitHub

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

Jump to bottom

1st step to build SF as shared library #4626

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

1st step to build SF as shared library #4626

wants to merge 7 commits into from

Conversation

ab-chesspad
Copy link

I would still want to be able to build SF as a shared library. It gives certain benefits to the user-developer, e.g. easy usage, flexibility, etc.
To achieve this I need to make three main changes to SF:

  1. Entry points to invoke SF:
    launch/initialize
    quit
    execute any SF function
    read SF output
    read SF error output
  2. Read commands and write output using std:cin and std:cout only in the standalone mode. Provide async mechanizm for input/output in the shared library mode.
  3. 'Non-stop' execution. SF must not crash and stop only when getting 'quit' command. In case of an error SF reports it and rolls back to the previous state.

And of course, all SF functionality and performance must be preserved.
I have implemented all these goals in my SF fork, but after a glitch during sync, I lost all the info. So now I am starting from scratch, using my local repo, this time in stages, so that people can easily review and analyze my submissions.
This is the first part of step 1:
Create the entry point to execute any SF command for the future shared library.
Thank you
Alex Bootman

@PGG106
Copy link

PGG106 commented Jun 15, 2023

I'm not sure opening pull requests, something meant to merge this changes into the main StockFish repository, makes sense.
You are better off working on your own Repository.

@MinetaS
Copy link
Contributor

MinetaS commented Jun 15, 2023

Stockfish is a UCI chess engine. I don't see there are major reasons to introduce shared-library-compatible features. People can fork the repository and do their stuffs individually, and I believe this one also falls in such category.

@UniQP
Copy link
Contributor

UniQP commented Jun 15, 2023 •
edited

I have implemented all these goals in my SF fork, but after a glitch during sync, I lost all the info.

I don't know if it helps in your case but if you've committed your changes, you should still find them via git reflog.

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 15, 2023 via email

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 15, 2023 via email

@PGG106
Copy link

PGG106 commented Jun 15, 2023

you can implement whatever you want in whatever way you want, it just makes no sense to try to PR said changes into SF

@ab-chesspad
Copy link
Author

If it makes sense to allow building SF for various platforms from the same repo, then consider this shared library version as an option to build SF for Android. Starting with Android 10 it's not allowed to use a program as a separate process.
I already saw this opposition when I tried to suggest it two years ago. I really do not understand why you people are so against making SF more flexible and convenient?

@PGG106
Copy link

PGG106 commented Jun 15, 2023 •
edited

one thing is running a CI to build a binary, another is PR-ing a code change, you are really just better off working on this on your own repo, that being said i'm not the maintainer so you might want to wait for him to respond (but i think this is a pretty clear case)

Edit: and even if you want to get merged you didn't do your due diligence, is this change functional? did run an sprt on fishtest? just opening a PR isn't enough

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 15, 2023 via email

@PGG106
Copy link

PGG106 commented Jun 15, 2023

not relevant to anything anyone said to you up to now. Good luck with your future endeavors

@RogerThiede
Copy link

Can you explain more why the current implementation of Stockfish will become incompatible with later versions of Android OS? You'll have a better pitch for updating the implementation of Stockfish to not branch the community of third party applications which rely on Stockfish for Android users.

In my experience, when I've seen users think they need a shared library to make use of Stockfish, they instead settle on wrappers which provide a Stockfish API like https://pypi.python.org/pypi/stockfish or https://github.com/cutechess/cutechess.

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 15, 2023 via email

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 15, 2023 via email

@vondele
Copy link
Member

vondele commented Jun 16, 2023

so, my 2cents on this discussion. It might be needed to provide SF as a shared library if we want to continue to be available on e.g. the app stores as part of GUIs. There might be other advantages, e.g. in data generation where the UCI overhead is not small, etc.

One could imagine to rewrite SF such that the main + uci functionality is a light wrapper around a library. This is a major project. Technically, I haven't looked at the current PR code. So, I don't know if it goes in the right or the wrong direction. Designing a proper interface for a shared library is something that needs care and thought, and possibly some input by experienced SF devs.

I do not intend to merge something like this in the current development cycle, but having a proposal worked out for the next cycle might be a plan. A decision to integrate can only be made once complete code is available, and as usual will be based on an understanding of the advantages and disadvantages of the actual implementation.

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 16, 2023 via email

@ab-chesspad
Copy link
Author

ab-chesspad commented Jun 16, 2023 via email

@ddobbelaere
Copy link
Contributor

ddobbelaere commented Jun 16, 2023 •
edited

I think it's an interesting idea to factor out the core Stockfish functionality as a library and provide the Stockfish binary as a thin wrapper around it, as @vondele alludes to. To do it "correctly" in order that other projects (including "Stockfish official" projects) can make use of it is no small feat. Some things that come to mind:

  1. we desparately need to move to a more professional build system (e.g. CMake), to make some stuff a lot easier and prevents lots of boilerplate code
  2. we'd better use semantic versioning for the library part (major.minor.patch) to make it clear if we break the API at some point.
  3. what functionality do we want to expose? I would guess not every single function/class in the Stockfish namespace. This would imply separating public/private headers.
  4. Do we provide a C compatible interface? If not, (e.g. C++11 only) life would be a lot easier I think.

I think 1. and 2. are a must, 3. and 4. "nice to haves" (e.g. just make all the headers files public for 3.)

@Sopel97
Copy link
Member

Sopel97 commented Jun 21, 2023 •
edited

Just to leave a note. I think this approach is a good choice. It would keep the communication simple and standard. It shouldn't be problematic to use, and we can always add some higher level wrappers.

Ideally the interface should use C ABI though. They should also be explicitly exported, as on some platforms no functions are exported by default.

@ab-chesspad
Merge branch 'official-stockfish:master' into master
52f5992
@ab-chesspad
To make SF testing easier I added a new "wait <sec>" command in debug…
e3fc0e0 
… mode. This will allow using

a "script" as SF input, e.g.
./stockfish <../test
and something like
wait 5
gives SF a chance to execute previous command(s).
Also
'#' at the beginning is considered a comment and ignored.
@ab-chesspad
compatibility with MINGW
5696afe
@ab-chesspad
Copy link
Author

Please note that ABI will conflict with UCI. One of my goals is to maintain it.

@RogerThiede
Copy link

Why would adopting a C ABI conflict with using UCI? We could easily have an application that uses the shared library via C ABI and consumes UCI text via standard input, sockets, or other communication channel and outputs valid UCI text.

@ab-chesspad
Copy link
Author

I think it will be better if the library itself can be used with UCI without additional layers. Especially when its output is in the text form, so text in - text out.

@ab-chesspad
wrap std::cout and std::cerr with macros sync_cout and sync_cerr for …
ab6576b 
…easier future transition

to string-based output
@lucasart
Copy link

lucasart commented Jul 8, 2023 •
edited

I think what @vondele suggest is excellent design. SF itself could be reduced to a library, without UCI. And UCI would be an independent program (which can be a simple python script, or even written in a different compiled language such as Rust). This opens the door to implement CECP (for those who care - I don't - but I know they still exist), without soiling the SF code base. And of course to interact more directly with SF ABI without paying for the cost of UCI (pipe I/O and task switching).

@vondele vondele mentioned this pull request Aug 22, 2023
Open
@Disservin Disservin mentioned this pull request Nov 28, 2023
Open
@dubslow
Copy link
Contributor

dubslow commented Dec 10, 2023

I agree on the following two points:

  1. making SF a library will have enormous benefit to the project, for foreseeable applications (android or other mobile apps), and for unforeseeable applications. We never know what is lost by failing to be as flexible as a library is.

  2. UCI should not be part of the core library. The main SF project should of course include the UCI interface that uses the core library, but the library, the engine, should always be separate from any interfaces (including the standard UCI interface).

@Disservin Disservin mentioned this pull request Jan 5, 2024
Closed
@Disservin Disservin mentioned this pull request Mar 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet