[mod] make static.build.commit more robust

[return42]

What does this PR do?

[mod] make static.build.commit more robust

• use single quote in the STATIC_BUILT_PATHS to avoid bash globbing
• don't try to commit if no files have been changed

How to test this PR locally?

Run make static.build.commit if nothing has been changed, you should see a "no changes applied / nothing to commit" message:

STATIC    build & commit /static files
...
STATIC    no changes applied / nothing to commit

Author's checklist

Related issues

[dalf]

If I try to call make static.build.commit the built commit is changed each time, here the reason:

• the previous build commit is dropped:

  searxng/utils/lib_static.sh

  Line 104 in 58a6738

  static.build.drop &>/dev/null

• then then themes are built
• $(git diff --name-only --cached) will always returns the list of built files

A possible workaround:

• build the themes
• check the diff, and continue if there is an actual diff
• drop the previous built commit
• commit

But I don't understand when this is useful.

[return42]

But I don't understand when this is useful.
..

• $(git diff --name-only --cached) will always returns the list of built files

Very complicate to reproduce the issue I addressed, because there is a commit (patch) noise .. see #566 (comment) and https://github.com/searxng/searxng/commits/master/searx/static/themes/oscar/css/logicodev.min.css.map

If this patch noise from oscar disappears one day, a themes.all might result in none changes.

By example: if you then checkout master branch, don't modify any file and just run make static.build.commit, then you will see that this make ends with an error.

To simulate the issue, checkout current master branch and comment out themes.all:

iff --git a/utils/lib_static.sh b/utils/lib_static.sh
index 2a5efeda..a905a9b7 100755
--- a/utils/lib_static.sh
+++ b/utils/lib_static.sh
@@ -105,7 +105,7 @@ static.build.commit() {
 
     (   set -e
         # build the themes
-        themes.all
+        #themes.all
 
         # add build files
         for built_path in "${STATIC_BUILT_PATHS[@]}"; do

commit this patch and then run make static.build.commit ... then you will see a confusing output .. coming from a git-commit while no (modified) file has been added before ...

$ LANG=C make static.build.commit 
STATIC    build & commit /static files
On branch master
Your branch is ahead of 'upstream/master' by 1 commit.
  (use "git push" to publish your local commits)

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	MEMO.txt
	test-respone.html
	test123.py
	test123.sh

nothing added to commit but untracked files present (use "git add" to track)     <<<--- !!!!!
make: *** [Makefile:99: static.build.commit] Error 1

[dalf]

With changes in the source code

• initial git history:
  • source V1
  • build V1
  • source V2

make static.build.commit step by step:

• static.build.drop does nothing
  • source V1
  • build V1
  • source V2
• themes.all
  • source V1
  • build V1
  • source V2
  • build V2 (not commited)
• $(git diff --name-only --cached) checks if there are some changes: yes
• git commit:
  • source V1
  • build V1
  • source V2
  • build V2 (commited)

👍

With no change in source code

• initial git history:
  • source V1
  • build V1
  • source V2
  • build V2

make static.build.commit step by step:

• static.build.drop drops "build V2":
  • source V1
  • build V1
  • source V2
• themes.all
  • source V1
  • build V1
  • source V2
  • build V2 (not commited)
• $(git diff --name-only --cached) checks if there are some changes: yes (the comparison is done with build V1)
  • source V1
  • build V1
  • source V2
  • build V2 bis (same diff than build V2 but it is a new commit)

Do I miss something?

[dalf]

If the purpose is to produce the same build for the same source including the .map files here a patch that fixes the issue (after 5 compilations there is no change to commit, so I guess the issue is fixed).

patch

diff --git a/searx/static/themes/oscar/gruntfile.js b/searx/static/themes/oscar/gruntfile.js
index 8e118afd..f5ee68b8 100644
--- a/searx/static/themes/oscar/gruntfile.js
+++ b/searx/static/themes/oscar/gruntfile.js
@@ -107,7 +107,7 @@ module.exports = function(grunt) {
         options: {
           paths: ["src/less/pointhi", "src/less/logicodev", "src/less/logicodev-dark", "src/less/bootstrap"],
           plugins: [
-            new (require('less-plugin-clean-css'))()
+            new (require('@wikimedia/less-plugin-clean-css'))()
           ],
           sourceMap: true,
           sourceMapURL: (name) => { const s = name.split('/'); return s[s.length - 1] + '.map';},
diff --git a/searx/static/themes/oscar/package.json b/searx/static/themes/oscar/package.json
index ea522b9d..db84af70 100644
--- a/searx/static/themes/oscar/package.json
+++ b/searx/static/themes/oscar/package.json
@@ -10,7 +10,7 @@
         "grunt-contrib-watch": "~1.1.0",
         "jslint": "^0.12.1",
         "less": "^4.1.1",
-        "less-plugin-clean-css": "^1.5.1"
+        "@wikimedia/less-plugin-clean-css": "^1.5.2"
     },
     "dependencies": {
         "bootstrap": "^3.4.1",
diff --git a/searx/static/themes/simple/gruntfile.js b/searx/static/themes/simple/gruntfile.js
index ca59cb56..bc5f2d88 100644
--- a/searx/static/themes/simple/gruntfile.js
+++ b/searx/static/themes/simple/gruntfile.js
@@ -108,7 +108,7 @@ module.exports = function(grunt) {
         options: {
           paths: ["less"],
           plugins: [
-            new (require('less-plugin-clean-css'))()
+            new (require('@wikimedia/less-plugin-clean-css'))()
           ],
           sourceMap: true,
           sourceMapURL: (name) => { const s = name.split('/'); return s[s.length - 1] + '.map';},
diff --git a/searx/static/themes/simple/package.json b/searx/static/themes/simple/package.json
index 356230f5..55c5de06 100644
--- a/searx/static/themes/simple/package.json
+++ b/searx/static/themes/simple/package.json
@@ -14,7 +14,7 @@
     "grunt-image": "^6.4.0",
     "ionicons": "^6.0.0",
     "less": "^4.1.1",
-    "less-plugin-clean-css": "^1.5.1",
+    "@wikimedia/less-plugin-clean-css": "^1.5.2",
     "sharp": "^0.29.3",
     "stylelint": "^13.13.1",
     "stylelint-config-standard": "^22.0.0",

It uses @wikimedia/less-plugin-clean-css instead of less-plugin-clean-css --> the compilation is done with clean-css version "^4.2.1" instead of the old version "^3.0.1".

[return42]

If the purpose is to produce the same build for the same source including

No, this is not the intention of this PR / this might be the topic of an other PR

static.build.drop drops "build V2":

static.build.drop drops only local builds / that makes the scenario more complex I think.

But intention of the new added condition is very simple:

        # check if any file has been added (in case of no changes)
        if [ -z "$(git diff --name-only --cached)" ]; then
            build_msg STATIC "no changes applied / nothing to commit"
            return 0
        fi

No matter what the scenario of the context is, if non files has been added, it does not make sense to try a commit.

$(git diff --name-only --cached) checks if there are some changes

AFAIK this command is used to print out added (staged) files .. see: https://git-scm.com/docs/git-diff#Documentation/git-diff.txt-emgitdiffemltoptionsgt--cached--merge-baseltcommitgt--ltpathgt82308203

Maybe it is better to use the alias option --staged to make it more clear .. but this option is not very common.

If you have a doubt in this PR, you can mark this PR as a Draft .. later when oscar is removed, we will see, that this PR fixes an issue .. and then, we can decide to merge / or not.

[dalf]

I still don't understand when this happen, but

No matter what the scenario of the context is, if non files has been added, it does not make sense to try a commit.

ok 👍