(build): Use Sapling HEAD for sl version by default in make oss#440
(build): Use Sapling HEAD for sl version by default in make oss#440vegerot wants to merge 1 commit into
make oss#440Conversation
vegerot
force-pushed
the
pr440
branch
2 times, most recently
from
e5c303c to
0101d5a
Compare
Summary: (build): Support sapling in tag-name.sh Currently, tag-name.sh only works with git. This is fine, because it's only used in GitHub CI, but I also want to use this script for local builds (which will be in a future commit). This commit will use either sl or git for creating tag names. Test-Plan: Automated testing blocked on #447 ```sh ❯ ./ci/tag-name.sh 0.2.20230107-190233-hc1336fcb ``` --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/sapling/pull/439). * #440 * __->__ #439 Pull Request resolved: #439 Reviewed By: quark-zju Differential Revision: D42611397 Pulled By: bolinfest fbshipit-source-id: d182ae3027ca72933aa2f4118e12d3e04453dec1
sggutier
left a comment
sggutier
left a comment
There was a problem hiding this comment.
Thanks for the interest on changing the versioning schema for debugging purposes.
What if instead of the changes you made here you modify the pickversion function on setup.py?
You could change it so that it uses sl instead of hg if it is available on the OSS build (or maybe even git).
| HGNAME ?= hg | ||
| SL_NAME = sl | ||
|
|
||
| export SAPLING_VERSION ?= $(shell ../../ci/tag-name.sh) |
There was a problem hiding this comment.
Internally we use a different naming schema, so we cannot set this environment variable on the Makefile itself. Besides, ../../ci/tag-name.sh only really exists on the GitHub repo, and not in our internal one, which makes setting this variable also troublesome.
There was a problem hiding this comment.
Thank you for the feedback. Updated! What do you think?
| rpmbuild \ | ||
| --define "sapling_root `pwd`" \ | ||
| --define "version $(VERSION)" \ | ||
| --define "version $(SAPLING_VERSION)" \ |
There was a problem hiding this comment.
We cannot change this either, as the VERSION environment variable is part of our internal CI.
Summary: Currently, following the recommended documentation for building Sapling will always set the sapling version to the upstream Mercurial version (which has been 4.4.2 for years) ```sh ❯ sl --version Sapling 4.4.2 ``` This isn't very useful when debugging Sapling, and is inconsistenct with the distributed versions of sl. This commit aligns the sl version with the sl version in the distributed versions of sl Test Plan: manual ```sh ❯ (cd eden/scm && make oss && ./sl --version) Sapling 0.2.20230107-190820-h75ace1ad ```
|
Looks better now (and I would really like to approve it), but there is still one troublesome issue (also commented in #522 ) |
| env["SystemRoot"] = os.environ["SystemRoot"] | ||
| return env | ||
|
|
||
|
|
There was a problem hiding this comment.
Prescribing a VCS without first detecting the repo type (hg, sl, git) still bugs me out. Could something be done for detecting the repo type?
Also, users having both hg and sl installed might be more common than one might think.
There was a problem hiding this comment.
I think this is an improvement. Previously setup.py hardcodes "hg" which obviously won't work in all cases. Whether we detect .hg or .sl or just try out both of them and pick one that works seems to be just an implementation detail to achieve the same goal, and I don't think that should block this PR.
|
@quark-zju has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: feat(build): Use git to set sl version number When building Sapling using `make oss`, the version number is only set to the date and commit when in an sl or hg repo. This commit adds the version number when Sapling is cloned with git Pull Request resolved: #522 Test Plan: $ git clone git@github.com:facebook/sapling.git # apply this patch $ cd sapling/eden/scm $ make oss $ ./sl --version On my machine: Sapling 4.4.2_20230204-131549-hf454128c --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/sapling/pull/522). * __->__ #522 * #440 Reviewed By: sggutier Differential Revision: D43326857 Pulled By: quark-zju fbshipit-source-id: 7a701f762f1199df0e6190dc59d4351208e58c17
4 participants
(build): Use Sapling HEAD for sl version by default in
make ossSummary: Currently, following the recommended documentation for building
Sapling will always set the sapling version to the upstream Mercurial version
(which has been 4.4.2 for years)
This isn't very useful when debugging Sapling, and is inconsistenct with the
distributed versions of sl. This commit aligns the sl version with the sl
version in the distributed versions of sl
Test Plan: manual
Stack created with Sapling. Best reviewed with ReviewStack.
make oss#440