-
Notifications
You must be signed in to change notification settings - Fork 5
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
7 build nsis khiops nocode installer in the ci #74
7 build nsis khiops nocode installer in the ci #74
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remarques sur le test de l'installeur:
- j'ai downloadé un fichier zip (khiops-installer.zip), et non un exe d'installeur
- une fois dézippé, j'ai obtenu un exe au nom tres "technique": khiops-10.1.5-SNAPSHOT-c184f8b-setup.exe, avec ensuite des messages d'installation bases sur ce nom (pas vraiment user friendly)
- lors de l'installation, j'ai eu "editeur inconnu": l'exe ne semble pas signe
- le message concernant l'installation de MPI donne une version tres detaillee de la version
Remarques sur l'installation:
- j'ai un fichier LICENCE (et non LICENCE.txt): on ne peut inspecter son contenu par un simple clic sur le fichier
- disparition des fichiers et repertoires suivants:
- README.txt
- install.txt
- whatsnewV10.1.txt
- doc
- samples
Remarques sur l'executable:
- le panneau d'aide (Help/Documentation), pour Khiops et Khiops coclustering référence toujours la doc et les samples
Remarque sur la procédure d'installation
- le README.md ne semble pas suffisant pour reproduire l'installation: l'objectif est que n'importe qui soit autonome pour pouvoir maintenir l'installeur, en maintenance corrective ou évolutives
- prérequis: l'outil NSIS doit etre installe
- ou se trouve les installeurs prerequis (java, MPI, visu): "somewhere in your system"???
- poser les installeurs java et MPI sur un file system partage?
- référencer les repos des outils de visualisation?
- comment gerer la signature de l'installeur (a automatiser a terme)
- comment gerer efficacement le cycle de developpement/test de l'installeur sur sa machine locale, avant de passer via un docker pour le CI/CD?
Thanks for the feedback @marcboulle below my answers.
This is normal as you downloaded an "artifact". When we will orchestrate the CICD process to make a release there will be no zip file. This was just to test.
This is done on purpose. When we release a true version there will be no
Yes, we still don't have signatures in the CICD.
Ok, I will simplify the version.
Ok, I will fix it
Ok, I will fix it.
This is very redundant with the contents of the web page. And if the user already installed it why he would care about install. I think just point to the website is enough. People rarely go see the documentation in the folder.
Ok, I will add it
I was under the impression that we will ask the user to download the samples ?
Ok, I will fix it, but I need an answer for the samples.
Ok I will detail better the procedure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few things to update, and things to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLSample is a lib not a binary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, but I find useful to have the parallel-samples
target. It helped me to find the source of problems of MPI in Mac. I would keep the new target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is PLTest for this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Done.
.github/workflows/pack-nsis.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the test job, it would be good to run Standard/Adult like it is done on Linux Cf. test-khiops action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we should discuss this, SNAPSHOT is a maven/java thing (as far as I know). Here this is not the version of khiops but the name of the installer. It is a big difference and we should discuss it : what about linux packages ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed SNAPSHOT
to preview
. The preview
particle appears when there is no tag or when the a tag is not coherent with the version in the sources.
Je viens de voir Nicolas. Pour la V10, les répertoire suivants peuvent être traités pour être au plus proche du comportement actuel (suite à discussion avec Nicolas), en évitant d'avoir des des répertoires en écritures dans ProgramFiles:
Pour la V11, on pourra revoir ça si nécessaire. |
67011c3
to
0bba5ba
Compare
Il manque un endroit pour stocker les docs de l'outil:
Une solution possible (à spécifier)
|
78435d7
to
de414d0
Compare
Les fichiers |
include(GoogleTest) | ||
enable_testing() | ||
|
||
# Add testing targets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Add testing projects" is more accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i'll put Add testing sub-projects
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just few things
src/Norm/CMakeLists.txt
Outdated
@@ -7,31 +7,19 @@ add_subdirectory(genere) | |||
add_subdirectory(basetest) | |||
add_subdirectory(generetest) | |||
|
|||
# build norm.jar | |||
# Add norm.jar target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne suis pas d'accord... l'idée ici est bien de construire un jar. ça definit effectivement une nouvelle cible, mais c'est pas vraiment ce qui nous intéresse. En l'occurence la cible n'est pas norm.jar
mais norm_jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every target has an associated "built object". CMake is oriented to targets and what we do in the CMakeLists.txt
files is defining and configuring targets.
I can expand the comment : "Add norm_jar target (which builds norm.jar)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non je ne suis pas d'accord. Le commentaire initial est plus clair pour tout le monde
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a potential newcomer (as I was a few months ago) it is more informative to say what CMake usually does in a CMakeLists.txt
scripts: to setup targets. I have not found any "build" methods in CMakeLists.txt directives, only add_*
and set_*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to Add norm.jar (CMake names the target norm_jar)
Also fixes #28 |
de414d0
to
7b57687
Compare
f7a0542
to
0d60b59
Compare
0ad5ac0
to
bbb89b6
Compare
bbb89b6
to
265686b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concernant le suffixe preview, on peut le laisser pour l'instant dans la version windows, mais créer une issue qui:
- pose la question de l'intérêt de ce suffixe
- si ok: homégénisation sur toutes les platformes (linux...)
- si ko: on revient à avant
L'alternative (supression ou commentaire maintenant, puis issue) est plus coûteuse à mettre en place, et ne changera rien au statut après discussion de l'issue.
- Add path filtering so it only executs when relevant code is changed - Use temporarily phoenix-actions/test-reporting until dorny/test-report fix a bug showing the dashboard in other workflows - Improve readability of test steps
A pull_request event is better suited than push event for our purposes. A push event is triggered not only when pushing but when merging via the GH web interface.
265686b
to
b9937a1
Compare
The khiops.jar contains only icons/images. They were in the "empty" namespace but the CMake add_jar directive does not work well when the namespace is empty. The solution is to always use a namespace for the jar images. Additionally the add_jar code was refactored to add_khiops_jar function in the top CMakeLists.txt
The position independent code settings are since CMake 3.14 handled with the POSITION_INDEPENDENT_CODE target property. This automatically sets the proper linker flags to achieve this objective for all supported toolchains.
I created issue #100 for this subject. |
4c8291a
to
858e73b
Compare
- Add and readapt the NSIS scripts - All bundled executables are now passed as parameters within the makensis call - Add a workflow that build the NSIS packages in the GH CI - Update samples location in documentation - Update readme and whatsnew files
858e73b
to
8b25cdc
Compare
This PR implements the NSIS installer creation with Github workflow. The workflow is set to be executed manually at the moment and can generate an installer from any branch or tag.
I also added two minor fixes:
src
(@marcboulle could check those)Also @marcboulle if you could test the installer generated (https://github.com/KhiopsML/khiops/suites/15487352576/artifacts/884460194)