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

Update and improve BUILDING.md #239

Open
Korne127 opened this issue Nov 25, 2024 · 5 comments · May be fixed by #242
Open

Update and improve BUILDING.md #239

Korne127 opened this issue Nov 25, 2024 · 5 comments · May be fixed by #242

Comments

@Korne127
Copy link

The building markdown file is outdated (CentOS hasn't been maintained in the last three years), incomplete (there is no tutorial for Arch-based distributions) and misses some packages. It seems like the packages someone needed to install on their system were just written down, but if you run the given commands on a fresh Ubuntu instance, many packages miss, e.g. GPGME, nlohmann json, argagg, boost and many more.

It would be good to update it and improve it to include a complete tutorial on how to build it.

probonopd added a commit that referenced this issue Nov 25, 2024
Everything in the yml file for ease of understanding. No Docker.

For #239
@probonopd
Copy link
Member

probonopd commented Nov 25, 2024

This project uses Docker to build the binaries, and unfortunately this results in the build process not fully self contained/self documenting in build.yml. I am trying to build it on Ubuntu without Docker. Looks like BUILD.md is not complete indeed.

https://github.com/AppImageCommunity/AppImageUpdate/blob/main/ci/Dockerfile.x86_64 seems to be the key.

(@TheAssassin: For this reason, I always like to have a self-contained .yml file in my projects that build the project, with no other files involved.)

@TheAssassin
Copy link
Member

TheAssassin commented Nov 25, 2024

There is no reason to maintain a building README in my opinion. We don't do that elsewhere either. And I certainly don't want to do it.

@Korne127
Copy link
Author

Ohhh thank you, that is good to know.
And I can understand not wanting to maintain a building file, but I think if you do, it should be accurate.
If there is no specific building readme file, the main building steps are usually explained in the core README.md file though.

I wouldn't remove these steps from the repository:

git clone --recursive https://github.com/AppImage/AppImageUpdate
cd AppImageUpdate/

mkdir build/
cd build/

cmake -DBUILD_QT_UI=OFF -DCMAKE_INSTALL_PREFIX=/usr ..
make -j$(nproc)

sudo make install

as they're not self-explanatory and pretty important for people without experience building this project to get that done.

@TheAssassin
Copy link
Member

And I can understand not wanting to maintain a building file, but I think if you do, it should be accurate.

I agree. I'd rather remove it. In my opinion, the Dockerfile sufficiently documents the necessary requirements for a certain distribution. Some dependencies we build ourselves (e.g., googletest) can be used just fine from the distribution on newer releases.

sudo make install is not an endorsed way to install AppImageUpdate in any way. We do not support distribution-wide installations. I think what would work the easiest is to instruct people how to run ci/build.sh resp. ci/build-in-docker.sh.

@probonopd probonopd linked a pull request Nov 26, 2024 that will close this issue
@Korne127
Copy link
Author

Korne127 commented Dec 8, 2024

I think it's important to include some tutorial on how to build this. I can understand not wanting to maintain a separate file, especially because it might get lost, but I think it should then be put in the README.md (as Readmes usually contain a building section).

I personally also didn't notice the Dockerfile. I did look at the workflow but couldn't get any insight from that, so at least the Dockerfile should be explicitly linked in the Readme.

The whole reason why I looked at this is because I'm updating the AppImage documentation and I was looking at / working on the updating part. And several ways to update currently require to build this project as there are no pre-built versions of libappimageupdate or libappimageupdate-qt. I think this itself is the bigger problem; these things should be provided in the releases (as I said in #238), but as long as this isn't the case, it's important to at least find the information on how to build it (and currently, it doesn't even exist for libappimageupdate-qt at all).

While I understand not wanting to include information for specific systems that is likely to be outdated soon, the main steps (which I posted as a block comment above) are absolutely necessary for a non-C++/make developer to build this and shouldn't be removed.

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 a pull request may close this issue.

3 participants