BuddiesOfBudgie / budgie-desktop-view

Budgie Desktop View is the official Budgie desktop icons application / implementation.
Apache License 2.0
29 stars 8 forks source link

Cannot build from source - git check fails #16

Closed Azania closed 1 year ago

Azania commented 1 year ago

1.2.1 added 'check: true' on https://github.com/BuddiesOfBudgie/budgie-desktop-view/blob/a763515af60d199e709b23ba65e76f26b27eb4ff/meson.build#L46 but this is causing it unable to build if you are not building from git. If you download the source from the site by a tarball e.g. on the release page you can't build as the check will cause the build to fail. if I remove the check: true it works fine so it is an issue for sure.

Build error is: --- stderr --- fatal: not a git repository (or any parent up to mount point /var/tmp)"

Edit: From what I can see there is basically 3 ways to fix this;

1) remove the 'check: true' part 2) change the if statement check on whether git is present on the system to check if the directory is a git repo or something 3) remove/uninstall git from the entire system

Obviously number 3 is not ideal :P

JoshStrobl commented 1 year ago

If you download the source from the site by a tarball e.g. on the release page you can't build as the check will cause the build to fail.

Are you downloading the release tarballs or the "source" ones? If you aren't downloading the release ones, you are downloading the incorrect tarballs and should be using git instead.

Azania commented 1 year ago

I am the package maintainer for Gentoo. For our packages we grab the source and then build/compile it on the system itself. So yes I grab the source tarball and compile that. This is normal for gentoo. However while making the packages (we call them ebuilds) for budgie 10.7 and other software parts this was the only one that failed. With the if git.found() check and then calling the command with check: true you are forcing that if somoene builds this from source it must be in a git repo and it doesn't even check what is around this. Could be any git repo as it just checks the HEAD rev.

This also prevents anyone from tinkering or forking (just as an example) unable to just grab the source and build it for their system if they have git installed. For example you have a compile option for stateless, what if someone wants to build with that but doesn't want to initialize the git repo but just grab the source and build it? You are preventing that if the user has git installed on their system, whether it was used or not. While gentoo does support git builds of course that also puts me in an awkward position where I am have to set git are a build dependency requirement just cause of this check. For my package I currently patch out the check: true part to bring it back in line with the 1.1.1 release. But I could just patch out the entire git check option instead to make it more clean. Also Gentoo isn't the only source based distro around. Imho this is an issue, and should be fixed.

fossfreedom commented 1 year ago

@Azania plase can you clarify why you are not using the release tarball which is added to the assets?

JoshStrobl commented 1 year ago

You should be using release tarballs. Git builds should be cloning the git repo, at which point they have git. This is a non-issue IMO.

JoshStrobl commented 1 year ago

Also Gentoo isn't the only source based distro around.

Arch handles this just fine with budgie-desktop-view-git by cloning git, which is the expected behavior.

Azania commented 1 year ago

@Azania plase can you clarify why you are not using the release tarball which is added to the assets?

You should be using release tarballs. Git builds should be cloning the git repo, at which point they have git. This is a non-issue IMO.

Even using the release tarball will result in the error, unable to build. As meson still checks if the build is inside a cloned git repo. Why would you release it in tarball and then the contents of that still have the git cloning required? It doesn't make any sense to me?

Also the tarball I was grabbing is https://github.com/BuddiesOfBudgie/budgie-desktop-view/archive/v1.2.1.tar.gz . But as said, whether I grab the tarball from the release one in the assets or the from archive result is the same if it is not in a cloned repo directory. What is the point of the release tarballs?

Also Gentoo isn't the only source based distro around.

Arch handles this just fine with budgie-desktop-view-git by cloning git, which is the expected behavior.

They require git since they clone it and build it then, thus bypassing this. This is the only package from BuddiesOfBudgie that requires to be build from a cloned gitrepo even. Not Budgie-Desktop, not Budgie-Control-Center etc, why is that? In gentoo git is by default not installed (last I checked) and tries to be minimalistic (and in general succeeds). Only packages that do not have release or source tarballs where git is the only option require git, nothing else.

Is it really needed to add the git head version to the package version or otherwise it can't be build? Or is there another reason that if you want to build it you need to be in the cloned git repo? I can see why you add check: true on that line so there are no issues with adding the git version, but shouldn't the git.found() be ammended to add if it is in a cloned repo rather than check if git is installed on the system itself? And include the git rev the tarball was made of instead of doing a git rev-parse HEAD command then if it is not in a git cloned repo directory?

Added is the build log from meson where it shows at the bottom it is crashing.

meson-log.txt

JoshStrobl commented 1 year ago

Why would you release it in tarball and then the contents of that still have the git cloning required? It doesn't make any sense to me?

It doesn't. The meson is very explicit:

git = find_program('git', required: false)

Followed by:

if git.found()

None if the git command bits run if it cannot find the program. This is identical to how it is in budgie-desktop. See:

Also the tarball I was grabbing is https://github.com/BuddiesOfBudgie/budgie-desktop-view/archive/v1.2.1.tar.gz .

That isn't the release tarball. That is the tarball generated by GitHub.

What is the point of the release tarballs?

Asset bundling, typically for translations and git submodules, which is useful for non-networked builds.

This is the only package from BuddiesOfBudgie that requires to be build from a cloned gitrepo even.

It does not require to be cloned from a git repo. It doesn't work that way in Fedora, Debian, or Solus. Seems like either your build is the issue here, or Gentoo.

Is it really needed to add the git head version to the package version or otherwise it can't be build?

No. version is declared in the project. head is only added if git is found.

Your log indicates:

Program git found: YES (/usr/bin/git)

So it will reasonably assume it is a git repo, as there is no other reason why you should have git in the build environment.

JoshStrobl commented 1 year ago

I'm going to close this issue as it is quite clear from the log that the build environment in question has git, in which case our expectation (including in budgie-desktop) is that the build is git. This enables the package version to be appended with the git commit sha of HEAD. Remove git from the build environment and use the correct tarball, or otherwise use git cloning.

Azania commented 1 year ago

Why would you release it in tarball and then the contents of that still have the git cloning required? It doesn't make any sense to me?

It doesn't. The meson is very explicit:

git = find_program('git', required: false)

Followed by:

if git.found()

None if the git command bits run if it cannot find the program.

That has nothing to do with any git repository.

This is identical to how it is in budgie-desktop. See:

No, not the same. Check line after 70, 71. https://github.com/BuddiesOfBudgie/budgie-desktop/blob/e59a2003d4e27d5b2ced7ce12747901446edfb15/meson.build#L71 check: false instead of check: true. Which means it won't crash if the directory or its parents are not in a cloned git repo. Budgie-desktop -> check: false Budgie-desktop-view -> check: true

Also the tarball I was grabbing is https://github.com/BuddiesOfBudgie/budgie-desktop-view/archive/v1.2.1.tar.gz .

That isn't the release tarball. That is the tarball generated by GitHub.

I was using that tarball and when I switched to directly using the tarball from your release page as you said, it still crashed. And I also said I did use https://github.com/BuddiesOfBudgie/budgie-desktop-view/releases/download/v1.2.1/budgie-desktop-view-v1.2.1.tar.xz as well but the result is exactly the same. The log I posted is from using that release tarball. Not from any archive or anything else.

What is the point of the release tarballs?

Asset bundling, typically for translations and git submodules, which is useful for non-networked builds.

It would still crash then if run as a module from a non cloned project that uses this as a subproject as it still has to build it then.

This is the only package from BuddiesOfBudgie that requires to be build from a cloned gitrepo even.

It does not require to be cloned from a git repo. It doesn't work that way in Fedora, Debian, or Solus. Seems like either your build is the issue here, or Gentoo.

Is it really needed to add the git head version to the package version or otherwise it can't be build?

No. version is declared in the project. head is only added if git is found.

I can have git installed on a system with 0 projects and still would still crash. Cause that is what find_program() does, not if the directory or parent belongs to any git project as there is no .git directory there with the information.

Your log indicates:

Program git found: YES (/usr/bin/git)

So it will reasonably assume it is a git repo, as there is no other reason why you should have git in the build environment.

No that is not how it works. The log sais that cause of

git = find_program('git', required: false) If it finds git installed on the system or not it will output that in the log as expected. So that is normal. It just checks if git is installed on the system.. I have git installed on the system, so it is yes. Not whether it is in a directory of subdirectory from a cloned git repository. That is a big difference. What happens is as follows:

-> checks for git, found, post in log -> checks if git was found; -> If yes: runs 'git rev-parse HEAD' command and checks if return is true, aka there was no error. -> if there was no error great, ammend the git head revision to the package version -> if there was an error.. build crashes due got git fatal error. -> If no: nothing happens and continues.

The difference with budgie-desktop is that the check: false part instead of check: true part. It won't check if the result was true or not. cause a fatal error by git returns an error code so the check returns false if check: true was set. Which for budgie-desktop it is set to false, whereby for budgie-desktop-view it is set to true.

The biggest issue here from what you wrote is that you make a wrong assumption here:

So it will reasonably assume it is a git repo, as there is no other reason why you should have git in the build environment.

Which is not the case. the program_find only checks if git is installed on the system. I just run with git removed from my system and only then it says in log: Program git found: NO. Which I can supply if you want.

I'm going to close this issue as it is quite clear from the log that the build environment in question has git, in which case our expectation (including in budgie-desktop) is that the build is git. This enables the package version to be appended with the git commit sha of HEAD. Remove git from the build environment and use the correct tarball, or otherwise use git cloning.

As said before. This statement is wrong. That is not what find_program('git', required: false) does at all. Which means the following that the build is git is also wrong. Yes the environment has git. I can install and uninstall it np from my entire system. So you are basically telling me to either make the package git cloning rather than using a release tarball for 1 package while the rest of budgie doesn't need it or force people to uninstall git from their system to compile this package from tarball.

If you say it is the same as budgie-desktop then you need to change the check: true to check: false. Simple as that. Cause atm they are not the same but different, and causing a crash when using the release tarball.

JoshStrobl commented 1 year ago

So you are basically telling me to either make the package git cloning rather than using a release tarball for 1 package while the rest of budgie doesn't need it or force people to uninstall git from their system to compile this package from tarball.

The expectation is that if they are building it on their system, they should be either using an isolated build environment where they are either:

  1. Using the release tarball, in which case git does not need to be in the environment
  2. Using git.

Or on their host system, where it is expected that the user would be developing Budgie Desktop (or using git builds, like with the -git build that I referenced).

That is not what find_program('git', required: false) does at all.

I never claimed it did. That is what the rev-parse does. I am fully aware what find_program does, just as I am of the check on the run_command.

Per the Meson doc:

If check: true is given, meson will error out if command returns with a non-zero exit code. Alternatively, you can set check: false and get the exit code with r.returncode().

The check is expected behavior, the fact it works for budgie-desktop is purely accidental in nature given the intent behind the builds in the scenarios I provided above.


NOTE: I will change it regardless, just know that you are not using suggested methods for building our components.

JoshStrobl commented 1 year ago

This has been modified in git main branch

Azania commented 1 year ago

NOTE: I will change it regardless, just know that you are not using suggested methods for building our components.

Thank you. That fix works. And I will take note of what you said.

JoshStrobl commented 1 year ago

@Azania Does Gentoo have a mechanism for building these things in a chroot that could be promoted as the way to build Budgie components "for users" (when they don't get it from the repo, which isn't the case for source-based OSes obviously)? For example, Fedora has mock and Solus has solbuild.

Azania commented 1 year ago

I would have to dig into portage to see how it works and how it builds all the paths and stuff used for building packages on the system. I am not sure if they are using chroot when building or not. I do know that everything tha is build is inside /var/tmp/portage/// etc. There is also /etc/portage/ for additional configuration etc inc make.conf which is used for building. In short I can't tell you right now for sure. A lot of variables and paths get added and sent over to build. Even compiler options set in the make.conf file i mentioned earlier gets added to variables for the compilers. So it is isolated at the very least from what I gather, but am not an expert on portage inner detailed workings. I have to look it up etc and go in deep.

eli-schwartz commented 1 year ago

@JoshStrobl

This entire logic is invalid, checking for the presence of a git program doesn't inform Meson whether the project is building from a git clone or from an uploaded meson dist tarball.

The thing that tells you whether the program is building from a meson dist tarball is:

fs = import('fs')

if fs.exists('.git')
    message('running from a git repo; will run git to determine stuff like versions')
    git = find_program('git', required: true) # it's a git clone, git must be installed
    version = run_command(git, 'describe', '--importantflags', check: true)
endif

It's also possible to use "the success of running git with check: false" instead of testing for the presence of a file or directory named .git (file in the case of a worktree), but it's really one or the other...

The deprecation notice for not specifying "check" is because meson can't know whether it's good or bad to check, not because everyone must unconditionally use "true".

Of course, if it's the intention to ban the use of meson dist tarballs then by all means, use the check kwarg to materialize that intention.

If you're going to make the check required, then if process.returncode() == 0 is unneeded and in fact, dead code which cannot ever evaluate to something other than 0. Keeping it in implies that there's an expectation it might reasonably be failing and that's okay, the build should still succeed.

JoshStrobl commented 1 year ago

@eli-schwartz Yea that's a fair point and probably a better way of implementing it. PRs welcome.

eli-schwartz commented 1 year ago

Can do, later today if no one beats me to it.

eli-schwartz commented 1 year ago

17

eli-schwartz commented 1 year ago

I did the fix I suggested above for budgie-desktop itself, here: https://github.com/BuddiesOfBudgie/budgie-desktop/pull/294