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

Move JS directory inside reactpy to fix broken source distributions #1183

Closed

Conversation

thegamecracks
Copy link

Issues

#1180

Solution

This PR moves the JS directory inside reactpy and tweaks the build process so hatch can produce source distributions that contain the JS client's source code necessary for end users to build the package.
Due to this being a major change in the project structure, developers will need to adapt their workflows to the new location of js/.

Checklist

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.

@Archmonger
Copy link
Contributor

Your original comment mentions that a symlink has issues, but could a hard link work instead of moving the source folder?

@thegamecracks
Copy link
Author

Your original comment mentions that a symlink has issues, but could a hard link work instead of moving the source folder?

@Archmonger As far as I know, most operating systems don't support hard linking directories. You could hard link all the individual source files instead, but it seems that Git doesn't distinguish hard links from normal files so they can't be preserved in version control.

@Archmonger
Copy link
Contributor

Archmonger commented Jan 9, 2024

I'd say we hard link all files recursively, and then have a .gitignore for the entire hardlinked directory (src/py/reactpy/js/*).

Burying the JavaScript source within the Python source feels awkward to me for something that's fundamentally just a build tools problem.

@thegamecracks
Copy link
Author

thegamecracks commented Jan 9, 2024

That has similar issues to js/ being force-include'd since it appears hatch doesn't have a clear way to only affect the sdist and source installations while skipping wheels being built from sdist. Perhaps a single hatch_build.py could be written with some reliable way to detect this and copy accordingly?

As for avoiding intermixed Python and JavaScript source directories, I think the difficulties in configuring the build system stems from it not sufficiently supporting file structures that go past the root of pyproject.toml, which is what determines the sdist archive root. Given that reactpy is the main package of the project, an alternative to moving the sources around could be placing reactpy/pyproject.toml at the repository root so the build system has an easier time including both source directories.

@Archmonger
Copy link
Contributor

Archmonger commented Jan 9, 2024

I've discussed with @rmorshea in the past of removing Hatch from this repo's workflow. There wasn't any issues with the old setuptools installation method, and the reason we switched to hatch was the concept of a monorepo.

But the monorepo never happened, so this feels like added complexity and bugs for little gain.

@rmorshea
Copy link
Collaborator

rmorshea commented Jan 9, 2024

Is there any downside to just not shipping an sdist, or at least delaying this change? I can work on switching back to a setuptools-based workflow but it'll just be fairly tedious. Some of the scripts are dependent on the current repo structure so unfortunately it won't be as simple as just moving this directory around.

@thegamecracks
Copy link
Author

thegamecracks commented Jan 9, 2024

That's a fair approach too. Based on the packaging guide, the risks of not publishing an sdist would be:

  1. Preventing users from building reactpy for their own system
  2. Potentially losing the project source code if the original repository is lost

The first point matters less to reactpy since it has a pure-python wheel, but if some future change makes the build process dependent on system architecture, it becomes extra important for reactpy (or possibly a third-party) to provide wheels for every system architecture that its users might have, perhaps with a tool like cibuildwheel. The second point depends on how easily users can access the repository, which I think is probably going to be fine for the forseeable future.

Another way to fix the sdist could be to update tasks.py so it hard links the files before installation, and also include some "relink" command to repeat the same hard linking before proceeding with building/publishing, combining the pyproject.toml changes and @Archmonger's suggestion together. The limitations change from having the source code intermixed in version control to a more complicated, but still automated build process, which seems like a better tradeoff in regards to the monorepo structure. Either way, I don't disagree with dropping the sdist or going back to setuptools.

@Archmonger Archmonger linked an issue Jan 10, 2024 that may be closed by this pull request
@kumaraguru1735
Copy link

@thegamecracks , I already told they will not accept others PR, Just a deadly project this is ...

@Archmonger
Copy link
Contributor

@kumaraguru1735 right now Ryan is the only one with permission to approve PRs for this repo, but he hasn't had time to do any OSS.

It's okay to criticize things moving slow, but the toxicity isn't appreciated.

@kumaraguru1735
Copy link

@Archmonger , why not you don't have permission for PR ? You are team of Reactpy wright ?

@kumaraguru1735
Copy link

@Archmonger , if he fails to give permission, I think you are not part of the game! I hope you will create seperate repo and put on own ideas and Maintain it

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 4, 2024

@Archmonger has been granted admin privileges now.

@rmorshea
Copy link
Collaborator

rmorshea commented Feb 4, 2024

I'm working on a solution to this that won't involve moving the directory. As per @thegamecracks's suggestion just linking the JS directory and including it as an sdist-only artifact should be sufficient.

@rmorshea rmorshea mentioned this pull request Feb 4, 2024
2 tasks
@Archmonger
Copy link
Contributor

@rmorshea I think you might also have to set me as an organization owner. I can't change workflow settings, which prevents me from changing required workflows.

@Archmonger
Copy link
Contributor

Closing this since Ryan is pushing for #1191

@Archmonger Archmonger closed this Feb 14, 2024
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.

NotADirectoryError when installing from source distribution
4 participants