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

replace shell scripts with x.py #725

Merged
merged 17 commits into from
Jul 12, 2022
Merged

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Jul 11, 2022

See also #692 (comment)

tisonkun added 12 commits July 11, 2022 23:26
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun marked this pull request as ready for review July 12, 2022 02:38
@tisonkun tisonkun requested review from PragmaTwice and git-hulk July 12, 2022 02:38
x.py Outdated
Comment on lines 72 to 80
find_command("autoconf", msg="autoconf is required to build jemalloc")
cmake = find_command("cmake", msg="CMake is required")

output = run([cmake, "-version"], stdout=subprocess.PIPE)
output = run(["head", "-n", "1"], stdin=output, stdout=subprocess.PIPE)
output = run(["sed", "s/[^0-9.]*//g"], stdin=output, stdout=subprocess.PIPE)
cmake_version = output.read().decode().strip()
if semver.compare(cmake_version, CMAKE_REQUIRE_VERSION) < 0:
raise RuntimeError(f"CMake {CMAKE_REQUIRE_VERSION} or higher is required, got: {cmake_version}")
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't download CMake here if it's missing. Since it causes issue like #712 and I'd prefer to let the user explicit download such significant and complex configurable dependency.

Anyway, if we don't automatically download autoconf, why CMake?

x.py Outdated Show resolved Hide resolved
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested a review from PragmaTwice July 12, 2022 09:28
tisonkun added 2 commits July 12, 2022 17:29
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

tisonkun commented Jul 12, 2022

Look like we run out of OSS only credit on Travis CI and all pending jobs (at least of Kvrocks) are hanging to be queued. I'm asking INFRA on Slack for help.

UPDATE - It seems something wrong with Travis CI. Waiting for verification :(

Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Twice <twice@apache.org>
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

LGTM

cmake_require_version = '.'.join(map(str, CMAKE_REQUIRE_VERSION))
cmake_semver = SEMVER_REGEX.match(cmake_version)
if cmake_semver is None:
raise RuntimeError(f"CMake {cmake_require_version} or higher is required, got: {cmake_version}")
Copy link
Member

@PragmaTwice PragmaTwice Jul 12, 2022

Choose a reason for hiding this comment

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

I think we can provide a URL for users and developers to download suitable cmake, like https://github.com/Kitware/CMake/releases or https://cmake.org/download/

Copy link
Member

@PragmaTwice PragmaTwice Jul 12, 2022

Choose a reason for hiding this comment

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

Also, I think it would be good to provide a --cmake-bin to customize the use of the cmake executable.

These can be done in subsequent PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's too cumbersome to convey possible download links. Still we don't recommend for autoconf and users should be easily to search how to download CMake. For example, as a macOS user I'd prefer to manage CMake via Homebrew instead of download from either links provided here.

It's too much to a downstream project heuristically educating users to install a dependency.

Copy link
Member Author

@tisonkun tisonkun Jul 12, 2022

Choose a reason for hiding this comment

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

Also we already provide some preferred defaults about getting prerequisites in the README.md file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I think it would be good to provide a --cmake-bin to customize the use of the cmake executable.

Yes. This sounds reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@PragmaTwice since this patch merged, you may create an issue for --cmake-bin.

- name: Lint and check code
run: |
./cpplint.sh
./cppcheck.sh
./x.py check cpplint
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why do we call this file x.py?

Copy link
Member Author

@tisonkun tisonkun Jul 12, 2022

Choose a reason for hiding this comment

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

Naming is hard. I stolen the idea from rust-lang/rust to call the universal script entrypoint x.py.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks @tisonkun

@tisonkun
Copy link
Member Author

@PragmaTwice @git-hulk I'll run docker build . locally and merge this patch once it's OK.

Hope Travis CI can fix the queuing issue in time :(

@tisonkun
Copy link
Member Author

Merging...

@tisonkun tisonkun merged commit 8544c2f into apache:unstable Jul 12, 2022
@tisonkun tisonkun deleted the script-to-python branch July 12, 2022 14:43
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.

3 participants