feat: add ability to store different versions of plugin #58

dodokek · 2025-02-04T09:40:27Z

Additionally fix .so file naming libtest-plugin.so -> libtest_plugin.so

Closes #51, #34

lowitea · 2025-02-04T15:40:44Z

Я ещё не успел добавить соглашение про наименования коммитов, но давай, пожалуйста, соблюдать этот https://www.conventionalcommits.org/en/v1.0.0/

lowitea · 2025-02-04T15:43:05Z

src/main.rs

@@ -88,6 +88,9 @@ enum Plugin {
    },
    /// Alias for cargo build command
    Build {
+        #[arg(long, value_name = "TARGET_DIR", default_value = "target")]
+        target_dir: PathBuf,


Надо добавить в редми

lowitea · 2025-02-04T15:46:24Z

src/helpers/build/mod.rs

@@ -109,5 +137,7 @@ pub fn main(_params: &Params) {
        }
    }

+    // When plugin version is changed you MUST run cargo update before further use


Да вот нет, раз ты добавил тут перебилд при изменении Cargo.toml то не надо, тут стоит написать как раз что ты добавил это чтобы не делать cargo update

Хмм, я подумал сначала что мы не хотим злить людей, которые за выполнение пайплайна с cargo update, тогда поменяю да

lowitea · 2025-02-04T15:59:49Z

src/helpers/build/mod.rs

+    // Not proud with this code
+    Path::new(&env::var("OUT_DIR").unwrap())
+        .parent()
+        .unwrap()
+        .parent()
+        .unwrap()
+        .parent()
+        .unwrap()
+        .to_path_buf()


Должно примерно так сработать

Suggested change

// Not proud with this code

Path::new(&env::var("OUT_DIR").unwrap())

.parent()

.unwrap()

.parent()

.unwrap()

.parent()

.unwrap()

.to_path_buf()

Path::new(&env::var("OUT_DIR").unwrap())

.iter()

.nth_back(3)

.unwrap()

.into()

Почитал доку Path, есть схема работы с этой штукой

Path::new(&env::var("OUT_DIR").unwrap()) .ancestors() .take(4) .collect()

Огонь, а я этот метод не заметил

lowitea · 2025-02-04T16:00:46Z

src/helpers/build/mod.rs

-    Path::new(&manifest_dir_string)
-        .join("target")
-        .join(build_type)
+    // Get output path from env variable to get path up until debug/ or release/ folder


Это надо переместить в докстринг функции

lowitea · 2025-02-04T16:03:31Z

src/helpers/build/mod.rs

+    let pkg_version = env::var("CARGO_PKG_VERSION").unwrap();
+    let pkg_name = env::var("CARGO_PKG_NAME").unwrap();
+    let plugin_path = out_dir.join(&pkg_name).join(&pkg_version);
+    let lib_name = format!("lib{}.{}", pkg_name.replace('-', "_"), LIB_EXT);


nit

Suggested change

let lib_name = format!("lib{}.{}", pkg_name.replace('-', "_"), LIB_EXT);

let lib_name = format!("lib{}.{LIB_EXT}", pkg_name.replace('-', "_"));

lowitea · 2025-02-04T16:07:52Z

src/helpers/build/mod.rs

+
+        for plugin_artefact in fs::read_dir(&plugin_version_path).unwrap() {
+            let entry = plugin_artefact.unwrap();
+            if entry.file_type().unwrap().is_symlink() {


Предлагаю уменьшить вложенность и сделать "если не симлинт, тогда continue"

lowitea · 2025-02-04T16:12:23Z

src/helpers/build/mod.rs

+                if fs::exists(plugin_version_path.join(&lib_name)).unwrap() {
+                    fs::remove_file(plugin_version_path.join(&lib_name)).unwrap();
+                }
+                fs::copy(out_dir.join(&lib_name), plugin_version_path.join(&lib_name)).unwrap();
+
+                // Copy manifest file
+                if fs::exists(plugin_version_path.join("manifest.yaml")).unwrap() {
+                    fs::remove_file(plugin_version_path.join("manifest.yaml")).unwrap();
+                }


У тебя тут довольно странный код получается, логика странная, ты ищешь первую симлинку и если находишь, то начинаешь удалять остальные симлинки в папке. Надо просто проверить что найденная симлинка, это симлинка на бинарь, и тогда её удалить и заменить на файл. А на манифест вообще симлинки не надо делать, ты в билдскрипте его сам генеришь, надо просто всегда вместо симлинка копировать манифест в папку с версией

lowitea · 2025-02-04T16:13:51Z

src/helpers/build/mod.rs

+                if fs::exists(plugin_version_path.join("migrations")).unwrap() {
+                    fs::remove_file(plugin_version_path.join("migrations")).unwrap();
+                }


На миграции тоже симлинки не нужены, проще сразу их положить файлами в папку с версией

lowitea · 2025-02-04T16:14:53Z

src/commands/lib.rs

@@ -1,5 +1,6 @@
 use anyhow::{bail, Context, Result};


Не забудь поправить сообщение коммита

lowitea · 2025-02-05T08:17:26Z

Cargo.toml

@@ -36,6 +36,7 @@ nix = "0.29.0"
 colored = "3"
 rand = "0.9"
 derive_builder = "0.20"
+toml_edit = "0.22.23"


Это видимо должно быть в dev-dependencies

lowitea · 2025-02-05T11:09:41Z

src/helpers/build/mod.rs

+            // Need to remove symlink before copying in order to properly replace symlink with file
+            if fs::exists(plugin_version_path.join(&lib_name)).unwrap() {
+                fs::remove_file(plugin_version_path.join(&lib_name)).unwrap();
+            }
+            fs::copy(out_dir.join(&lib_name), plugin_version_path.join(&lib_name)).unwrap();


nit: можно было бы plugin_version_path.join(&lib_name) в переменную вынести

lowitea · 2025-02-05T11:10:32Z

src/helpers/build/mod.rs

+            // Need to remove symlink before copying in order to properly replace symlink with file
+            if fs::exists(plugin_version_path.join(&lib_name)).unwrap() {
+                fs::remove_file(plugin_version_path.join(&lib_name)).unwrap();
+            }
+            fs::copy(out_dir.join(&lib_name), plugin_version_path.join(&lib_name)).unwrap();


nit2: И кажется можно без if просто сразу попробовать удалить и забить на ошибку

lowitea · 2025-02-05T11:11:27Z

tests/run.rs

+    }
+
+    let mut plugin_creation_proc =
+        run_pike(vec!["plugin", "new", "test_plugin"], TESTS_DIR).unwrap();


Давай сделаем тире в имени проекта и дополнительно проверим что ссылка ссылается на валидное имя. Раз чинили тут это

Closes #51, #34

dodokek force-pushed the more_versions branch 3 times, most recently from aab5e0b to 566f605 Compare February 4, 2025 14:21

lowitea reviewed Feb 4, 2025

View reviewed changes

lowitea requested changes Feb 4, 2025

View reviewed changes

lowitea reviewed Feb 5, 2025

View reviewed changes

dodokek force-pushed the more_versions branch 3 times, most recently from 5ae6404 to 6270e99 Compare February 5, 2025 11:03

lowitea requested changes Feb 5, 2025

View reviewed changes

lowitea reviewed Feb 5, 2025

View reviewed changes

feat: add ability to store different versions of plugin

7f25aaf

Closes #51, #34

dodokek force-pushed the more_versions branch 3 times, most recently from 5fa5b41 to 55fd54a Compare February 5, 2025 12:00

dodokek added 3 commits February 5, 2025 15:36

test: add integrational tests for build.rs

854fffa

ci: change branch for pike in plugin template

adc7ca1

ci: change CI trigger for push

97db00c

dodokek force-pushed the more_versions branch from 55fd54a to 97db00c Compare February 5, 2025 12:36

lowitea approved these changes Feb 5, 2025

View reviewed changes

lowitea merged commit 7ca1983 into master Feb 5, 2025
4 checks passed

lowitea mentioned this pull request Feb 13, 2025

Как пользователь я хочу устанавливать несколько версий плагина в пикодату #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to store different versions of plugin #58

feat: add ability to store different versions of plugin #58

dodokek commented Feb 4, 2025

lowitea commented Feb 4, 2025

lowitea Feb 4, 2025

lowitea Feb 4, 2025

dodokek Feb 5, 2025

lowitea Feb 4, 2025 •

edited

Loading

dodokek Feb 5, 2025

lowitea Feb 5, 2025

lowitea Feb 4, 2025

lowitea Feb 4, 2025

lowitea Feb 4, 2025

lowitea Feb 4, 2025 •

edited

Loading

lowitea Feb 4, 2025

lowitea Feb 4, 2025

lowitea Feb 5, 2025

lowitea Feb 5, 2025

lowitea Feb 5, 2025

lowitea Feb 5, 2025

	let lib_name = format!("lib{}.{}", pkg_name.replace('-', "_"), LIB_EXT);
	let lib_name = format!("lib{}.{LIB_EXT}", pkg_name.replace('-', "_"));

feat: add ability to store different versions of plugin #58

feat: add ability to store different versions of plugin #58

Conversation

dodokek commented Feb 4, 2025

lowitea commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowitea Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowitea Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowitea Feb 4, 2025 •

edited

Loading

lowitea Feb 4, 2025 •

edited

Loading