Skip to content

Beta interpreteur #94

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

Open
wants to merge 5 commits into
base: beta
Choose a base branch
from
Open

Beta interpreteur #94

wants to merge 5 commits into from

Conversation

MrWaloo
Copy link

@MrWaloo MrWaloo commented Jun 26, 2025

Description

Permet de contourner le forçage de l'interpréteur par le plugin script pour les commandes info de type "script" afin de ne pas avoir d'erreur sur des certains systèmes et de permettre l'utilisation d'une version spécifique de l'interpréteur (python via pyenv et/ou dans un venv).

Suggested changelog entry

  • Correction de l'erreur d'affichage des options pour un type "HTTP" lors de la création d'une commande alors que le type "script" est sélectionné à ce moment.
  • Traitement différent pour les commandes de type Script : le forçage de l'interpréteur selon une règle propre au plugin et qui est incompatible avec certains systèmes (python3 sous Debian 11+) est conditionné afin que le script soit lancé comme s'il l'était dans un shell sauf si le forçage de l'interpréteur est nécessaire.
  • Mise à jour de la documentation et petites corrections de celle-ci

Related issues/external references

Fixes #

Types of changes

  • Bug fix (non-breaking change which fixes)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the GNU.
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added MD documentation for the sniff.

MrWaloo added 2 commits June 27, 2025 01:31
- Ajout de la possibilité de ne pas forcer l'interpréteur selon une règle propre à Jeedom et qui est incompatible avec certains systèmes (python3 sous Debian 11+).
- Correction de l'erreur d'affichage des options pour un type "HTTP" lors de la création d'une commande alors que le type "script" est sélectionné à ce moment.
@Mips2648 Mips2648 self-requested a review June 27, 2025 06:18
@Mips2648 Mips2648 requested a review from zoic21 June 27, 2025 06:19
@@ -229,13 +229,14 @@ public function execute($_options = null) {
return script::$_requet_cache[$request];
}
$cmd = 'sudo chmod +x ' . explode(' ', $request)[0] . ' 2>/dev/null;';
if (strpos($request, '.php') !== false) {
$force_interpreter = $this->getConfiguration('doNotForceInterpreter', '0') == '0';
Copy link

@kwizer15 kwizer15 Jul 7, 2025

Choose a reason for hiding this comment

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

Ça me gène quand on nomme quelque chose avec "doNot". On peut très vite partir sur des doubles négations et ça deviens très compliqué d’interpréter correctement la règle humainement.

exemple : $notOk !== false

Je serai partisant d’une option forceInterpreter.

Suggested change
$force_interpreter = $this->getConfiguration('doNotForceInterpreter', '0') == '0';
$force_interpreter = $this->getConfiguration('forceInterpreter', '1') != '0';

Comme ça on garde la même logique et la rétro-compatibilité.

Qu’est-ce que tu en penses ?

Copy link
Author

@MrWaloo MrWaloo Jul 7, 2025

Choose a reason for hiding this comment

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

Je dois avouer avoir hésité, je fais les corrections

edit: pour éviter de forcer l'option 'forceInterpreter' à 1 et donc de cocher la case par défaut, ce qui peut perturber les utilisateurs, je baptise le bouton 'useShebang'. Comme ça la case reste décochée, le test est simplement inversé :

$use_shebang = $this->getConfiguration('useShebang', '0') == '1';
if (!$use_shebang && strpos($request, '.php') !== false) {
	$cmd .= 'php ' . $request;
} elseif (!$use_shebang && strpos($request, '.rb') !== false) {
	$cmd .= 'ruby ' . $request;
} elseif (!$use_shebang && strpos($request, '.py') !== false) {
	$cmd .= 'python ' . $request;
} elseif (!$use_shebang && strpos($request, '.pl') !== false) {
	$cmd .= 'perl ' . $request;
} else {
	$cmd .= $request;
}

@MrWaloo MrWaloo requested a review from kwizer15 July 11, 2025 10:18
@zoic21
Copy link
Contributor

zoic21 commented Jul 14, 2025

Bonjour
Je viens de regarder et je me demande si ça serait pas plus simple de détecter dans le chemin du script si y’a besoin de mettre l’interpreteur ou pas. Par exemple si ça commence par /bin alors on met pas l’interpreteur. On pourrait aussi lire la première ligne du script voir si l’interpreteur est là et dans ce cas pas d’interpreteur non plus.

@kwizer15
Copy link

Bonjour
Je viens de regarder et je me demande si ça serait pas plus simple de détecter dans le chemin du script si y’a besoin de mettre l’interpreteur ou pas. Par exemple si ça commence par /bin alors on met pas l’interpreteur. On pourrait aussi lire la première ligne du script voir si l’interpreteur est là et dans ce cas pas d’interpreteur non plus.

Très bonne idée. Je plusois.

@MrWaloo
Copy link
Author

MrWaloo commented Jul 17, 2025

Bonjour
Je viens de regarder et je me demande si ça serait pas plus simple de détecter dans le chemin du script si y’a besoin de mettre l’interpreteur ou pas. Par exemple si ça commence par /bin alors on met pas l’interpreteur. On pourrait aussi lire la première ligne du script voir si l’interpreteur est là et dans ce cas pas d’interpreteur non plus.

Très bonne idée. Je plusois.

Effectivement, c'est la variante la plus propre et comme je sais que les gros changements de principe dans les plugins officiels sont évités, je n'avais pas voulu faire comme ça. Mais ça me plaît bien plus.

J'aimerais savoir ce qui est voulu :

  • suppression de la case à cocher "Interpréteur shebang"
  • utilisation d'un interpréteur forcé (comportement par défaut jusqu'à présent si on ne tient pas compte de cette PR) que si le script ne contient pas de ligne shebang ET le premier élément de la ligne de commande n'est pas un exécutable sous un chemin du PATH

Donc :

  • si le script contient une ligne shebang, le script est lancé tel quel comme si c'était dans un shell,
  • si le premier élément un un exécutable d'un des répertoire du PATH, la commande est exécutée tel quel comme si c'était dans un shell
  • sinon -> forçage de l'interpréteur en fonction de l'extension du script

J'insiste sur l'extension et pas le fait que le nom du script contient ".py" par exemple parce qu'un script "recuperation.php.py" est lancé aujourd'hui avec l'interpréteur php ce qui est un non-sens.

On est d'accord ? (en tout cas, je modifie dans ce sens).

@MrWaloo MrWaloo requested review from kwizer15 and zoic21 July 18, 2025 20:42
$first_element = explode(' ', $request)[0];
$use_shebang = false;
$from_path = false;
if (file_exists($first_element)) {

Choose a reason for hiding this comment

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

Je pinaille mais tu peux vérifier avec is_readable au lieu de file_exists. Ca permet de s'assurer aussi qu'on peut lire le fichier.

$env_path = explode(PATH_SEPARATOR, getenv('PATH'));
$from_path = in_array(dirname($first_element), $env_path);
if (!$from_path) {
$shebang = trim(file_get_contents($first_element, false, null, 0, 3));
Copy link

@kwizer15 kwizer15 Jul 19, 2025

Choose a reason for hiding this comment

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

Je trouve la méthode très lourde d'aller lire tout le fichier pour au final ne s'intéresser qu'à la première ligne.
C'est plus "complexe", mais utiliser fopen, fgets, fclose serait plus approprié. (ou si tu es intéressé tu peux regarder du côté de SplFile qui est natif et fait la même chose de façon orienté objet)
Ça te permettra de lire uniquement la première ligne et de stopper la lecture.

// Au lieu de file_get_contents avec limit
$handle = fopen($first_element, 'r');
if ($handle) {
    $first_line = fgets($handle);
    fclose($handle);
    $use_shebang = str_starts_with(trim($first_line), '#!/');
}

Copy link
Author

@MrWaloo MrWaloo Jul 20, 2025

Choose a reason for hiding this comment

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

Mis à part le fait que str_starts_with est une fonction php8, la fonction file_get_contents ne lit que ce qui lui est passé en paramètre. Là, c'est 3 caractères à partir du début qui seront lus.
https://www.php.net/manual/en/function.file-get-contents.php

Choose a reason for hiding this comment

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

Yes. C'était un juste exemple et c'est bien vue. J'avais pas fait gaffe aux paramètres offest et length. Du coup tu peux effectivement laisser comme ça.

$cmd = 'sudo chmod +x ' . $first_element . ' 2>/dev/null;';
}
}
if (!$from_path && !$use_shebang && substr($request, -4) === '.php') {
Copy link

@kwizer15 kwizer15 Jul 19, 2025

Choose a reason for hiding this comment

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

Ca serait plus joli avec un tableau associatif [extension => executable] comme la logique est la même.

$interpreters = [
    '.php' => 'php',
    '.py' => 'python', 
    '.pl' => 'perl',
    '.rb' => 'ruby'
];

$extension = substr($request, strrpos($request, '.'));
if (isset($interpreters[$extension]) && !$from_path && !$use_shebang) {
    $cmd .= $interpreters[$extension] . ' ' . $request;
}

@@ -6,6 +6,12 @@

- Support des images d'équipement personnalisées (Jeedom 4.5)

# xx/07/2025

Choose a reason for hiding this comment

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

@zoic21 me confirmera mais je crois que le changelog est complété lors des merges

Copy link
Author

@MrWaloo MrWaloo Jul 20, 2025

Choose a reason for hiding this comment

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

Ce n'est pas moi qui ai rajouté cette ligne

J'ai modifié le descriptif de la PR pour avoir le même changelog que dans le code, pour être sûr qu'il n'y ait qu'une seule version du changelog que je propose.

>
> - Si le script en première position de la ligne de commande contient une ligne shebang, le plugin script lancera un shell qui l’exécutera en se basant sur la directive de la 1ère ligne (shebang).
> - Si le premier élément de la ligne de commande est un exécutable reconnu par le système, par exemple `/usr/bin/python3`, le plugin script lancera un shell qui exécutera cette ligne de commande.
> - Si le script en première position de la ligne de commande ne contient pas de ligne shebang ET que le premier élément de laligne de commande n'est pas un exécutable reconnu par le système, l'extension de votre script doit absolument correspondre à son type. En effet le plugin script se base alors sur l'extension du script pour l'exécutable à lancer.
Copy link

@kwizer15 kwizer15 Jul 19, 2025

Choose a reason for hiding this comment

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

Manque un espace "laligne"

-ET que le premier élément de laligne de commande n'est pas un exécutable
+ET que le premier élément de la ligne de commande n'est pas un exécutable

@MrWaloo MrWaloo requested review from kwizer15 and Mips2648 July 20, 2025 14:38
@MrWaloo
Copy link
Author

MrWaloo commented Aug 5, 2025

Je cherche juste à comprendre et donc je pose la question : cette PR sera validée ?
Je sais qu'on est en période de vacances (je suis moi-même en congés) et c'est normal de prendre des jours off, je ne reproche rien à personne. C'est juste que là, après 2 semaines de silence radio, je me pose la question.

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.

4 participants