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

Add Python Source Distribution (sdist) support #457

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented May 11, 2024

This PR depends on #448, and will remain a draft until it merges, and can be rebased.


This PR adds experimental "Source Distribution" (or; sdist) support for python packaging. Basically, sdist distributions are all source files necessary to build a package, so that when pip can't find a pre-built binary, it will pull the sdist .tar.gz file, and attempt to build it itself.

As you can guess, this is prone to failure, and is only meant as a last-resort usecase, the CI conditions in #448 aren't present on all machines that Soar would want to be installed on, nor have all edgecases been enumerated in the SConstruct file, to support building on a variety of targets.

Yet, I wanted to try, and this PR - at least experimentally - allows for the creation of an sdist .tar.gz file when ran with python -m build Core/ClientSMLSWIG/Python --sdist.

This file can then be uploaded to pypi alongside the binary .whl distributions, or installed locally with pip install soar_sml-[...].tar.gz


There is an argument to be had about "Should we even have this? We don't support every single system that's out there" and that's a fair point, which is why I separated this from #448, to talk/discuss that, and also to extend the invitation of tons of testing on all sorts of systems.

If sdist files are uploaded for any version of soar-sml, pip will attempt to build it, when it has no binary release to grab.

This may result in errors (as the system does not have the right build tools, or other conditions preventing its success), and may result in extra support requests (or other similar issues).

@ShadowJonathan
Copy link
Contributor Author

Rebased on main.

This is more a discussion of "should we have this" instead of "we should have this" PR, I'm just bringing the prototype code/changes that can achieve it ^^

@ShadowJonathan ShadowJonathan marked this pull request as ready for review May 15, 2024 18:36
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented May 15, 2024

Roughly, you can use the following lines to test this;

pip install build

pushd Core/ClientSMLSWIG/Python

# -f to ignore if it doesnt exist
rm -f dist/*
pyproject-build . -vvvv -s
pip install dist/* -vvvv

popd

@garfieldnate
Copy link
Collaborator

Well, first what are the additional platforms that could be supported with this that might be interesting?

  • 32 bit Windows? Don't think it's relevant, honestly
  • ARM Windows and Linux? ARM is becoming more relevant, and Soar on Rasberry Pi could actually be nice
  • RTLinux?
  • RISC-V? Never used it before, don't know anyone that has.
  • An embedded system of some kind? But then you normally build on another system and deploy to the embedded one, and I don't know if this change covers that case.
  • Would also love a WebAssembly distro to run Soar in the browser, but again, the building platform and the deployment platform would be different, and I don't know if this supports that.

@ShadowJonathan
Copy link
Contributor Author

FTR, we could support most of those with wheel distributions.

Other platforms I could add to that list would be AIX / z/OS / freeBSD, etc.

I'd argue RISC-V would become more of an interest in the future ^^

WASM would be outside scope for python source packaging, imo that one could be better served with some rust-esc interface that people can then build upon and compile for WASM.

We could possibly attach this file to github releases, to provide people a way to install directly? Though, that would be almost indistinguishable from a full clone + local build.

An embedded system of some kind? But then you normally build on another system and deploy to the embedded one, and I don't know if this change covers that case.

Cross-compilation would be better done within CI and specialised environments, I don't know if this method would allow anyone to more effectively and easier re-package Soar, but since its a build method limited to python distribution, I doubt it.


I'll have to be honest, like this, it feels like this usecase is a solution looking for a problem. It is nice to define the sdist method, so people can be expected that it works, but since we might have a problem with releasing it as-is (because we want to prevent build errors on users' systems, and instead CI it for as many systems as possible), it might not be too useful.

Yeah, unless we want to make soar-sml buildable on systems, there are better ways to do this (such as the install-via-github command), and the only other use-case for this would be someone packaging soar-sml for an offline machine.


With that, I think adding the machinery for an sdist would maybe be useful for developers and researchers if they need it, but it doesn't solve a concrete problem at the moment.

# We cannot just add the directories, as they will then start to include the .tar.gz file,
# which scons treats as a "target", creating a circular dependency.
SOURCE_EXTS = {"h", "c", "cpp", "hpp", "cxx"}
for i in range(0, 5):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we'll ever go more than 5 deep, but if we did this would break and we might not notice. I would feel more comfortable with some logic that checks if any more files are being adde.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could just bump it from 5 to 10, would that suffice?

And add a CI job that does a basic sdist packaging + sdist-based build-and-install, possibly?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, 10 sounds fine. Testing in CI would also be a good idea.

@garfieldnate
Copy link
Collaborator

It doesn't look like a huge piece of implementation, honestly, so I wouldn't mind just adding it. Soar is the type of thing that might get used on a weird platform.

@ShadowJonathan
Copy link
Contributor Author

Let me then also add the appropriate documentation to explain such a use-case :)

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.

2 participants