-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Rewrite it in Rust #9512
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
Rewrite it in Rust #9512
Changes from all commits
d843b67
096b254
681a165
55f655f
e674678
f38543c
76adfed
c18fb74
60bd186
2dc2c8d
91be748
a502cb1
44d7540
517d53d
132d99a
83fd7ea
c2df63f
853649f
35083c7
cba03fc
cee1353
8b48373
8460b37
ba1c5d4
476b12e
7347c90
a446a16
dcca3cf
c8bf2be
f167ec9
39c3fae
d7febd4
0b160eb
cfb5bb2
4b85c2f
a16e2ec
bfa94bf
47cc98f
29a2c4b
958ad3a
4639f7e
8fd1db0
9ca160e
2581662
7f8d247
39f3c89
a8c9922
27c8845
3ed86fa
5a76c7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -148,6 +148,7 @@ Dependencies | |
|
||
Compiling fish requires: | ||
|
||
- Rust (version 1.67 or later) | ||
- a C++11 compiler (g++ 4.8 or later, or clang 3.3 or later) | ||
- CMake (version 3.5 or later) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the use of FindContent bumps CMake to 3.11 (unless corrosion gets included in the tree as discussed elsewhere) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Current versions of corrosion require at least CMake 3.15. Some features are only available with CMake 3.19 and newer. |
||
- a curses implementation such as ncurses (headers and libraries) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,49 @@ | ||||||||||||||
include(FetchContent) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to make builds using OBS difficult; is it possible to include Corrosion in the tree and use it as a subdirectory instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, distro packagers are not going to like this, but it's fine if you try
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Distro packager for Arch Linux here, this would make our lives so much easier with the ability to use system libraries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to add that, but my hope is that no distro packages fish while it uses Corrosion. This use is meant to be temporary during a single development cycle; by the next release I hope to have no CMake at all, and therefore no Corrosion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an ambitious goal. Good luck. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Corrosion can be added as a subdirectory/ submodule. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering the use of a fork of Corrosion, and that it is intended as a temporary measure that won't be released, I think using Corrosion as a subdirectory would make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello, throwing myself into this conversation with some additional info that distro maintainers might find useful. In this case, this would allow package maintainers to do Additionally, starting in CMake 3.24, it's possible to tell There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you, this is helpful. For Corrosion, my preferences are (in order): use FetchContent as in the PR (perhaps using bruxisma's tips), or add Corrosion as a fish-shell submodule, or directly check in Corrosion. Not sure what's best for OBS. We shouldn't use heroics to keep fish working on older platforms mid-port. That may mean turning off builders for certain platforms, and then turning them back on later when we're no longer obligated to support Corrosion/autocxx/etc. |
||||||||||||||
|
||||||||||||||
# Don't let Corrosion's tests interfere with ours. | ||||||||||||||
set(CORROSION_TESTS OFF CACHE BOOL "" FORCE) | ||||||||||||||
|
||||||||||||||
FetchContent_Declare( | ||||||||||||||
Corrosion | ||||||||||||||
GIT_REPOSITORY https://github.com/ridiculousfish/corrosion | ||||||||||||||
GIT_TAG fish | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably we want to have a SHA here, for reproducibility |
||||||||||||||
) | ||||||||||||||
|
||||||||||||||
FetchContent_MakeAvailable(Corrosion) | ||||||||||||||
|
||||||||||||||
set(fish_rust_target "fish-rust") | ||||||||||||||
|
||||||||||||||
set(fish_autocxx_gen_dir "${CMAKE_BINARY_DIR}/fish-autocxx-gen/") | ||||||||||||||
|
||||||||||||||
corrosion_import_crate( | ||||||||||||||
MANIFEST_PATH "${CMAKE_SOURCE_DIR}/fish-rust/Cargo.toml" | ||||||||||||||
FEATURES "fish-ffi-tests" | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
# We need the build dir because cxx puts our headers in there. | ||||||||||||||
# Corrosion doesn't expose the build dir, so poke where we shouldn't. | ||||||||||||||
if (Rust_CARGO_TARGET) | ||||||||||||||
set(rust_target_dir "${CMAKE_BINARY_DIR}/cargo/build/${_CORROSION_RUST_CARGO_TARGET}") | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please note that this is not always the correct path, depending on which Generator is used. Notably, it's not correct for Visual Studio generators and other Multi-config generators (e.g. Ninja-Multiconfig). On a related note, have you considered using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We don't support Visual Studio (or really Windows at all - it's either wsl or cygwin).
This seems to be a "ninja but you can do debug and release in one" thing? I think it would be fair to not support that until cmake is retired. The plan is to hopefully do that before the next release. Unless of course you know how to easily get the correct path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ignoring Visual Studio, the path currently is as below. This should be considered an implementation detail, but is also unlikely to change in the near future. if(CMAKE_CONFIGURATION_TYPES)
set (rust_target_dir "${CMAKE_BINARY_DIR}/$<CONFIG>/cargo/build/${_CORROSION_RUST_CARGO_TARGET}")
else()
set (rust_target_dir "${CMAKE_BINARY_DIR}/cargo/build/${_CORROSION_RUST_CARGO_TARGET}")
endif() |
||||||||||||||
else() | ||||||||||||||
set(rust_target_dir "${CMAKE_BINARY_DIR}/cargo/build/${_CORROSION_RUST_CARGO_HOST_TARGET}") | ||||||||||||||
corrosion_set_hostbuild(${fish_rust_target}) | ||||||||||||||
endif() | ||||||||||||||
|
||||||||||||||
# Tell Cargo where our build directory is so it can find config.h. | ||||||||||||||
corrosion_set_env_vars(${fish_rust_target} "FISH_BUILD_DIR=${CMAKE_BINARY_DIR}" "FISH_AUTOCXX_GEN_DIR=${fish_autocxx_gen_dir}" "FISH_RUST_TARGET_DIR=${rust_target_dir}") | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that the CMake script is intended as short-lived and soon-to-go-away — but this overlong 170-char line can be easily split:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I understand you mean well, and you have done a thorough review. But as it stands, this is distracting from the discussion in the PR (unfortunately github is terrible at displaying this). So I will be hiding this. Sorry! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, understood. No offense taken 🙌 |
||||||||||||||
|
||||||||||||||
target_include_directories(${fish_rust_target} INTERFACE | ||||||||||||||
"${rust_target_dir}/cxxbridge/${fish_rust_target}/src/" | ||||||||||||||
"${fish_autocxx_gen_dir}/include/" | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
# Tell fish what extra C++ files to compile. | ||||||||||||||
define_property( | ||||||||||||||
TARGET PROPERTY fish_extra_cpp_files | ||||||||||||||
BRIEF_DOCS "Extra C++ files to compile for fish." | ||||||||||||||
FULL_DOCS "Extra C++ files to compile for fish." | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
set_property(TARGET ${fish_rust_target} PROPERTY fish_extra_cpp_files | ||||||||||||||
"${fish_autocxx_gen_dir}/cxx/gen0.cxx" | ||||||||||||||
) |
Uh oh!
There was an error while loading. Please reload this page.