Skip to content
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

Add build scripts and optimized GitHub Actions #1860

Merged
merged 6 commits into from
Feb 10, 2025

Conversation

mr-manuel
Copy link
Collaborator

@mr-manuel mr-manuel commented Jan 17, 2025

Some script were added to make it much easier for everyone to locally build GUIv2 for a GX device and as WebAssembly. Currently it works only on X86 processors, due to currently used Emscripten (not for all versions ARM64 binaries are available) and Qt versions (only since 6.7.0 it appears that binaries are available to the community, else a license is required).

GitHub Actions were also modified to execute the new scripts, which makes maintenance less time consuming. For example the emscripten and QT version are now always fetched from an environment file and do not need to be updated in multiple scripts, GitHub Actions and wiki:

gui-v2/scripts/.env

Lines 1 to 3 in a4b7bc1

EMSCRIPTEN=3.1.37
QT_VERSION=6.6.3
OUTPUTDIR="/opt/venus/build-gx-hostedtoolcache"

I tested the scripts on:

  • Ubuntu 24.04 VM
  • Windows 11 with a fresh Ubuntu 24.10 WSL2 container
  • GitHub Actions
  • macOS 15 (Intel CPU) with Lima

After this PR is merged, also the wiki has to be modified and simplified.

I added also a .gitattributes file to fix automatically the line endings of .env and *.sh files, else they were always converted to CRLF instead of LF.

@mr-manuel mr-manuel requested a review from chriadam January 17, 2025 20:44
@mr-manuel mr-manuel force-pushed the mr-manuel/add-build-scripts branch 3 times, most recently from eb9a452 to bcab3f5 Compare January 20, 2025 14:32
@DanielMcInnes
Copy link
Contributor

DanielMcInnes commented Jan 20, 2025

Building the wasm version fails for old (20.04) ubuntu installations. Not an issue, but should probably be mentioned somewhere. I didn't capture a log, but it was to do with aqt needing a newer version of python than what you get with that old ubuntu.

Building the wasm version works fine on a fresh ubuntu 24.04 vm.

build-gx-install-requirements.sh fails on a fresh ubuntu 24.04 vm:

Downloading https://updates.victronenergy.com/feeds/venus/release/sdk/venus-dunfell-x86_64-arm-cortexa8hf-neon-toolchain-v3.53.sh
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  231M  100  231M    0     0  4487k      0  0:00:52  0:00:52 --:--:-- 4538k
sudo: venus-dunfell-x86_64-arm-cortexa8hf-neon-toolchain-v3.53.sh: command not found
ERROR: SDK installation failed.
dmcinnes@dmcinnes-System-Product-Name:~/git/victron/gui-v2$

I can see that the file was downloaded to /tmp, and if I run the command manually:
dmcinnes@dmcinnes-System-Product-Name:/tmp$ sudo ./venus-dunfell-x86_64-arm-cortexa8hf-neon-toolchain-v3.53.sh -y
... it works ok, so it's probably something simple to fix

build-wasm.sh Outdated Show resolved Hide resolved
build-wasm.sh Outdated Show resolved Hide resolved
@DanielMcInnes
Copy link
Contributor

DanielMcInnes commented Jan 20, 2025

If I manually install the toolchain:

dmcinnes@dmcinnes-System-Product-Name:/tmp$ sudo ./venus-dunfell-x86_64-arm-cortexa8hf-neon-toolchain-v3.53.sh -y
Victron Energy SDK installer version v3.53
==========================================
The directory "/opt/venus/dunfell-arm-cortexa8hf-neon" already contains a SDK for this architecture.
If you continue, existing files will be overwritten! Proceed [y/N]? Y
Extracting SDK.....................................................................done
Setting it up...done
SDK has been successfully set up and is ready to be used.
Each time you wish to use the SDK in a new shell session, you need to source the environment setup script e.g.
 $ . /opt/venus/dunfell-arm-cortexa8hf-neon/environment-setup-cortexa8hf-neon-ve-linux-gnueabi
dmcinnes@dmcinnes-System-Product-Name:/tmp$ cd
dmcinnes@dmcinnes-System-Product-Name:~$ cd git/victron/gui-v2/
dmcinnes@dmcinnes-System-Product-Name:~/git/victron/gui-v2$ ls
ApplicationContent.qml  build-gx-install-requirements.sh  build-gx.sh  build-wasm-install-requirements.sh  build-wasm.sh  cmake  CMakeLists.txt  CMakeLists.txt.user  components  data  fonts  FrameRateVisualizer.qml  gitdiff  Global.qml  i18n  images  LICENSE.txt  macdeployhelper.sh  Main.qml  nohup.out  pages  qqmlobjectmodel.cpp  README.md  scripts  src  test.py  tests  themes  tools  victron.astyle  windeployhelper.bat

... then source the environment setup stuff:
dmcinnes@dmcinnes-System-Product-Name:~/git/victron/gui-v2$ . /opt/venus/dunfell-arm-cortexa8hf-neon/environment-setup-cortexa8hf-neon-ve-linux-gnueabi

... then start the gx build, I get an error:

dmcinnes@dmcinnes-System-Product-Name:~/git/victron/gui-v2$ ./build-gx.sh
ERROR: Venus OS SDK was not found.
Execute "./build-gx-install-requirements.sh" once or visit this link for how to install and use the SDK: https://github.com/victronenergy/venus/wiki/howto-install-and-use-the-sdk
dmcinnes@dmcinnes-System-Product-Name:~/git/victron/gui-v2$ . /opt/venus/dunfell-arm-cortexa8hf-neon/environment-setup-cortexa8hf-neon-ve-linux-gnueabi 
dmcinnes@dmcinnes-System-Product-Name:~/git/victron/gui-v2$ 

@DanielMcInnes
Copy link
Contributor

I only have a linux box, I can't test this for mac or windows. Would you expect it to work on these platforms?

@blammit
Copy link
Contributor

blammit commented Jan 21, 2025

I understand these scripts make it easier to build, in terms of combining the package installation + configure + build steps, but should these scripts really be in the repo? It looks like they have various specific requirements/assumptions (e.g. Linux builds, doing clean builds, putting build dir inside the repo) so that it may often be easier to build without the scripts. Or, in other cases it is simple enough that you don't need a separate build script (e.g. build-gx.sh). I'm not sure how much benefit we get from adding these scripts, as opposed to having some improved build documentation, and this also creates additional tools that need to be maintained going forward.

@mr-manuel
Copy link
Collaborator Author

@DanielMcInnes if you install manually the Victron SDK you need to create a symlink for /opt/venus/current, which is currently described in the manual, therefore it does not work.

sudo: venus-dunfell-x86_64-arm-cortexa8hf-neon-toolchain-v3.53.sh: command not found

This sould already be fixed. It was because the command executed was sudo script.sh and not sudo ./script.sh.

I tested it on a fresh Ubuntu VM, on Windows with a WSL Ubuntu container and GitHub Actions. All mentioned use Ubuntu 24.04. The scripts won't work natively on Windows. For macOS I'm not sure, but don't think that it will work, since apt-get may be not available there.

@blammit the idea was to make it easy for community members to build the GUI and therefore also to make it attractive for developing and testing custom modifications. It's easy to build the GUI if you have QtCreator installed, but if you have not (like me and nearly all of the community), then this is quite a pain. It's also very complex to setup QtCreator correctly, see #492 and #1389.
To make those scripts easy to find I placed them in the root of the repository, but a subfolder with the proper name should also be fine.

This scripts also reduce maintenance for build scripts used in GitHub Actions, since all needed data is now fetched from the prepare script and those scripts are run to build the wasm file in the GitHub Actions.

From my point of view, this is a huge benefit, since:

  • It's more attractive for new people starting with development, since in future also apps should be added to Venus OS
  • The build documentation gets easily outdated and is not updated. It gets also easily forgotten
  • Before you needed to maintain the GitHub Actions script, which now is moved to these scripts. Therefore there is no additional work, but you need only to maintain other scripts. If they do not work anymore then you see that very fast under the GitHub Actions.

@mr-manuel mr-manuel force-pushed the mr-manuel/add-build-scripts branch from bcab3f5 to 618140b Compare January 21, 2025 14:04
@DanielMcInnes
Copy link
Contributor

DanielMcInnes commented Jan 27, 2025

I like the added convenience this brings to linux users, especially building and installing mqtt. I imagine similar scripts would be required for windows & mac users, resulting in 3 sets of build scripts that need to be maintained, which is not ideal. I think this would be better done via cmake, which would give you the same build behavior on all 3 platforms. See ExternalProject_Add at https://cmake.org/cmake/help/latest/module/ExternalProject.html . The downside of doing anything in cmake is the difficulty - it is often hugely frustrating.

For mqtt, you can do something like:

find_package(Qt6 Mqtt)
if qt6MqttNOTFOUND
ExternalProject_Add(....)
endif

@blammit
Copy link
Contributor

blammit commented Jan 28, 2025

@mr-manuel That's true it would be nice to build nicer than building with Qt Creator and downloading the SDK yourself, and much more convenient for community members. I think my main objection is that it looks a bit ad hoc in this current form. Unifying the builds under cmake as @DanielMcInnes suggested sounds like a good approach. What about using cmake presets? We could have presets like linux, wasm, windows, mac, and have standardized configure+build commands like:

cmake --preset wasm  # download/configure for wasm
cmake --build --preset wasm  # run the build

@mpvader
Copy link
Contributor

mpvader commented Jan 30, 2025

@blammit you wrote:

it would be nice to build with Qt Creator and downloading the SDK yourself, and much more convenient for community members.

Sorry but I don’t agree. Its the opposite. For people to try this out they need to be able to try this very simply without installing and setting up many things. Ie. change a qml file and run the build.

As long as the script works for our github actions + linux + linux on windows then I’m happy. And mac os is an extra nice to have.

By the way: all the rest of Venus OS is also oriented towards linux only; probably builds on Windows using its linux extensions, and might/or might not run on Mac OS; which is fine.

@mpvader
Copy link
Contributor

mpvader commented Jan 30, 2025

Wrt cmake: doesn’t that make it all a lot less readable? Lets keep it simple.

@mr-manuel mr-manuel force-pushed the mr-manuel/add-build-scripts branch 2 times, most recently from 5dcad48 to 240ad56 Compare January 30, 2025 22:04
@blammit
Copy link
Contributor

blammit commented Jan 31, 2025

Sorry but I don’t agree. Its the opposite. For people to try this out they need to be able to try this very simply without installing and setting up many things. Ie. change a qml file and run the build.

My mistake, I meant to say, "it would be nicer than building with Qt Creator...". I agree it's certainly easier for people to try it out if they don't need to download the tools and Qt Creator, etc. I'm just not sure adding build scripts to the repo is the way to go here. These would need to be regularly verified/maintained/supported, and if they don't work for different users' configurations then they will need to do their own hacking anyway. In that sense they seem more like templates for building. But maybe that will be obvious to community members and not really a problem there.

Wrt cmake: doesn’t that make it all a lot less readable? Lets keep it simple.

Yes it's definitely more work to use cmake, and not as simple as adding build scripts.

If we really need build scripts inside the repo then I'm happy to go ahead with it. @mr-manuel there are already scripts under scripts and tools. Maybe these can go under scripts/build? (Then perhaps eventually the deployment scripts could be moved from the root directory as well.)

@mr-manuel mr-manuel marked this pull request as draft January 31, 2025 07:04
@mr-manuel mr-manuel force-pushed the mr-manuel/add-build-scripts branch 10 times, most recently from 2d388c1 to c275618 Compare January 31, 2025 10:10
@mr-manuel
Copy link
Collaborator Author

I moved the scripts, added some notes and prepared them for supporting installation of the requirements on ARM64 architecture (like Mac's with M chip).

@dirkjanfaber as we wrote, I moved the POEditor scripts from scripts to .github/scripts.

@mr-manuel mr-manuel marked this pull request as ready for review January 31, 2025 10:12
@mr-manuel
Copy link
Collaborator Author

@blammit to make sure all scripts work correctly I can also add a GitHub Action that tests the scripts once they change or the .env changes. This way they will always work. This is also possible to do for ARM64 later, so all build possibilities are tested automatically.

@blammit
Copy link
Contributor

blammit commented Jan 31, 2025

@blammit to make sure all scripts work correctly I can also add a GitHub Action that tests the scripts once they change or the .env changes. This way they will always work. This is also possible to do for ARM64 later, so all build possibilities are tested automatically.

Yes that seems like a good idea, to make sure they are verified regularly.

Maybe these can go under scripts/build

Initially I was thinking this would be better as it would tidy up the root directory, but it does mean the scripts are harder to find, if we want to encourage community members to use them for building. I'm not entirely sure either way.

Note, the build instructions in the README should also be updated.

scripts/build-gx.sh Outdated Show resolved Hide resolved
scripts/build-wasm.sh Outdated Show resolved Hide resolved
@chriadam
Copy link
Contributor

chriadam commented Feb 5, 2025

Thanks! Looks great, for the most part. I think we still need the "manual" instructions in the wiki, to ensure that we support folks who don't use Ubuntu 24.04 or higher, and also because I think there is still value in desktop (Windows / Mac OS X) builds for fast-iteration testing.

@mr-manuel
Copy link
Collaborator Author

After this PR I would also add a short description on how to launch the build scripts on Windows and macOS. In your case you mean on how to build natively to start the GUI on Windows or macOS?

Copy link
Contributor

@DanielMcInnes DanielMcInnes left a comment

Choose a reason for hiding this comment

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

lgtm

@mr-manuel mr-manuel force-pushed the mr-manuel/add-build-scripts branch from c275618 to 023a960 Compare February 10, 2025 06:55
This is not needed anymore since #1801
@mr-manuel mr-manuel requested a review from chriadam February 10, 2025 07:07
@mr-manuel
Copy link
Collaborator Author

Rebase was done to include the changes to the index.html. You can now merge.

@DanielMcInnes DanielMcInnes merged commit 0598320 into main Feb 10, 2025
2 checks passed
@DanielMcInnes DanielMcInnes deleted the mr-manuel/add-build-scripts branch February 10, 2025 23:26
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.

5 participants