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

Fedora rpm changes #451

Closed
wants to merge 5 commits into from
Closed

Fedora rpm changes #451

wants to merge 5 commits into from

Conversation

kiilerix
Copy link
Contributor

Various minor changes to the sapling repo while hacking on Fedora packaging.

These changes works with https://gist.github.com/kiilerix/b32cb9688d2d6e9e752e91e2ca4c30e6 .

Still a long way to go to be able to package offline and reproducible.

Don't blame Mercurial for Eden/Sapling ;-)
There is a setup.py build_pyzip command to build edenscmdeps3.zip , but
the install target had a hard assumption that it had been built.

Avoid unnecessarily tight coupling.
setup.py build_py would also unconditionally run build_pyzip, even
though that also is a separate command that users can use if they
want it.

It is useful that the commands separate - don't hardcode them as one.
…ependencies

Network access is a potential problem for packaging and ensuring
reproducible builds. Packaging for distros might insist on using
packaged versions of external dependencies. Provide a way to opt out
from the setup.py fetching.

Note: Other parts of the build process will still access the network
(mainly crates.io , npmjs.com, and github.com).
@bolinfest
Copy link
Contributor

I haven't tested this yet, but I strongly suspect that changing HGNAME to sl instead of hg will break all of our internal stuff, so that's a non-starter.

@bolinfest
Copy link
Contributor

The other changes look safe, though!

@kiilerix
Copy link
Contributor Author

I haven't tested this yet, but I strongly suspect that changing HGNAME to sl instead of hg will break all of our internal stuff, so that's a non-starter.

Right. I don't have enough information about how you use it in your setup. I dropped the last two changesets. They were not critical - just an attempt at cleaning things up.

@facebook-github-bot
Copy link
Contributor

@quark-zju has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@quark-zju merged this pull request in 1abfcbc.

@quark-zju
Copy link
Contributor

@kiilerix I modified the environment variable to SAPLING_OFFLINE and restores the buildpyzip logic. Let me know if you run into issues.

@kiilerix
Copy link
Contributor Author

kiilerix commented Mar 3, 2023

Hi - Thanks for landing this.

It is surprising that the nice and clean history of the PR was squashed. I guess the PR was treated as what you seem to call a stacked change. I think it would be nice for acceptance by the wider the community that sapling both as tool and project also support the more standard workflow of just carefully using the tool as it kind of was intended.

@quark-zju
Copy link
Contributor

It is surprising that the nice and clean history of the PR was squashed.

Sorry for the bad experience. This is currently limited by the ImportIt tool (maintained by another team). It does not support importing multiple commits as multiple internal code review units.

The Sapling GitHub repo is a mirror of a subset of the internal repo, synced by the ShipIt tool. Internally one code review unit maps to one commit. We don't use the merge/rebase features that GitHub provides because this repo is a mirror.

Sapling itself is not involved in merging a PR.

I do think importing commits as a stack is better in this case. But that requires building extra tools that do not exist right now. Let me open up an issue on the ShipIt side to see what they think.

@kiilerix
Copy link
Contributor Author

kiilerix commented Mar 3, 2023

No problem. I'm just sharing the outside perspective. Thanks for considering it.

Also, interesting how ShipIt and sapling exposes the facebook-internal re-invention of https://arrenbrecht.ch/mercurial/pbranch/ ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants