yadm-dev / yadm

Yet Another Dotfiles Manager
https://yadm.io/
GNU General Public License v3.0
5.14k stars 177 forks source link

Env variables don't work in if-else-endif templates #488

Closed AtomToast closed 2 weeks ago

AtomToast commented 5 months ago

Describe the bug

Similarly to #486, trying to use env variables in {% if env.EXAMPLE == "example" %} with the default template processor does not work.

To reproduce

Can this be reproduced with the yadm/testbed docker image: Yes script.gz

Steps to reproduce the behavior:

  1. export TEST=testing
  2. cat << EOF > test.txt##template         
    {% if env.TEST == "testing" %}
    success
    {% else %}
    fail
    {% endif %}
    EOF
  3. yadm add test.txt##template
  4. cat test.txt

Expected behavior

The file should contain success, however instead it uses the fail branch.

Environment

Additional context

From a brief look at the code, it seems that this is actually just not implemented right now?

rasa commented 5 months ago

@AtomToast It's in the code at https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L429 and the test to confirm it works is at https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/test/test_unit_template_default.py#L246 Do other vars get substituted correctly?

rasa commented 5 months ago

On my system, env.HOME substitutes correctly. Ubuntu 24.04 and

$ yadm version
bash version 5.2.21(1)-release
 git version 2.43.0
yadm version 3.2.2
$ awk --version| head -n 1
GNU Awk 5.2.1, API 3.2, PMA Avon 8-g1, (GNU MPFR 4.2.1, GNU MP 6.3.0)
AtomToast commented 5 months ago

Please correct me if I'm wrong but to me this is again only the variable substitution part of the code. This works for me. What doesn't work for me are if blocks. As per https://yadm.io/docs/templates, in the if-else-endif section.

rasa commented 5 months ago

@AtomToast Good point. I haven't tested it, but the env. logic is the same as the other yadm. so I am really surprised it's not working. See https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L426-L431

AtomToast commented 5 months ago

For me this is 100% reproducable.

The code you linked is part of the replaces_vars() function, which does not seem to be called in the conditions() function. There all I can see is this https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L441 which I believe is the yadm. part of the logic? There is no EVIRON[label] call

rasa commented 5 months ago

@AtomToast You are correct. Just below https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L439-L444 we need to add

    for (label in ENVIRON) {
       value = ENVIRON[label] 
       pattern = sprintf("%s%s|", pattern, condition_helper(label, value)); 
    }
AtomToast commented 5 months ago

That alone seems to not have done it for me. Is there perhaps something else missing?

github-actions[bot] commented 3 months ago

This issue has been labeled as stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

AtomToast commented 3 months ago

@rasa Hey, sorry for the bother but the bot marked this as stale. Have you managed to make this work?

rasa commented 3 months ago

@AtomToast No sorry, I haven't. Hopefully @TheLocehiliosan can un-stale this, as I can't.

soraxas commented 3 months ago

@AtomToast Does these changes fixes for you?

https://github.com/soraxas/yadm/pull/1/files

diff --git a/yadm b/yadm
index bfbcd81..a61f37f 100755
--- a/yadm
+++ b/yadm
@@ -432,20 +432,23 @@ function replace_vars() {
 }
 function condition_helper(label, value) {
   gsub(/[\\.^$(){}\[\]|*+?]/, "\\\\&", value)
-  return sprintf("yadm\\.%s" blank "*==" blank "*\"%s\"", label, value)
+  return sprintf("%s" blank "*==" blank "*\"%s\"", label, value)
 }
 function conditions() {
   pattern = ifs blank "+("
+  for (label in ENVIRON) {
+    pattern = sprintf("%s%s|", pattern, condition_helper("env\\." label, ENVIRON[label]));
+  }
   for (label in c) {
     if (label != "class") {
       value = c[label]
-      pattern = sprintf("%s%s|", pattern, condition_helper(label, value));
+      pattern = sprintf("%s%s|", pattern, condition_helper("yadm\\." label, value));
     }
   }
   split(classes, cls_array, "\n")
   for (idx in cls_array) {
     value = cls_array[idx]
-    pattern = sprintf("%s%s|", pattern, condition_helper("class", value));
+    pattern = sprintf("%s%s|", pattern, condition_helper("yadm\\." "class", value));
   }
   sub(/\|$/, ")" blank "*%}$", pattern)
   return pattern

(I shall note that the current template replacement seems pretty inefficient as it needs to loop-through all class/envars everytime; though its still "fast enough" and the operation would be rare)

AtomToast commented 3 months ago

@soraxas Unfortunately no :/ I tried it with both the test case from the original post and some additional ones. I also tried cloning your entire repo which seems to have some additional changes.

Does it work for you with those changes?

soraxas commented 3 months ago

Yes it does works for me

image

Have you point your $PATH to your cloned repo (or maybe it's still using the older yadm)?

AtomToast commented 3 months ago

I first directly edited the yadm binary in /bin. When using the cloned repo I called yadm with an absolute path. For some reason, now having retested it on my laptop, it seems to work now. Not sure what I did wrong on my other PC or if there was some other variable