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

Variables not preserved between functions. #1006

Closed
bartoszek opened this issue Aug 23, 2019 · 14 comments
Closed

Variables not preserved between functions. #1006

bartoszek opened this issue Aug 23, 2019 · 14 comments

Comments

@bartoszek
Copy link

bartoszek commented Aug 23, 2019

I'm having this stupid issue with one of my packages (magma). Users keep reporting that binaries are installed in /usr/local/magma instead of /opt/magma path. In spite that PKGBUILD have explicitly stated CMAKE_INSTALL_PREFIX=/opt/magma in prepare() function.

I've just tested this in clean chroot, and it looks like yay in fact somehow is changing cmake install prefix is not preserving variables between prepare() and build(). This is really strange as plain makepkg is working as intended 😕

I'm not a yay user and a compleat noob in go, can't pin point the issue further...

@bartoszek bartoszek changed the title Yay variables not preserved between functions. Variables not preserved between functions. Aug 23, 2019
@Jguer
Copy link
Owner

Jguer commented Aug 23, 2019

Yay splits prepare() and build(), so it really is as you diagnose, it's a conscious choice for implementation. makepkg supports this type of operation, so it comes down to PKGBUILDs supporting it or not

@bartoszek
Copy link
Author

bartoszek commented Aug 23, 2019

Yep, but you could also drop --noextract --noprepare flags from last passToMakepkg and everyone would be happy. It isn't like you are altering sources like customizepkg 🤔
If I may, haven't you consider your build routine to be a bit abusive towards makepkg ?

@Morganamilo
Copy link
Contributor

If you dropped those flags then you'd end up with problems from prepare being ran twice (as well as the added time for extracting twice).

As far as I'm concerned pkgbuilds should declare their variables in every function where they need them and to do otherwise is incorrect.

Not doing so also breaks useful makepkg features like --repackage.

@bartoszek
Copy link
Author

bartoszek commented Aug 23, 2019

You've got a point there. Perhaps moving variable logic outside of functions would be a proffered solution, despite it's ugly and hacky 😞 and I will have a ton of packges to refactor

@Morganamilo
Copy link
Contributor

I didn't say outside of functions but instead inside each one. Not sure if there's a difference there but I believe that to be the recommended way to do it.

@bartoszek
Copy link
Author

And here lays the problem.

  • If you specify variable logick outside function (e.g _pyver=$(expac -Q "%v" python)). You can't use it with makepkg -s or aurutils without having expac preinstalled, but it would work with yay because of dependency resolver strictly following output from aur-rpc.
  • If you move it inside prepare(), it would work with makepkg and by extend aurutils but won't work with yay.
  • To make it work with all above you have to also put _pyver logic in in every function it gets utilized (prepare,build,package,package-doc)

As an aurutils user I tend toward second solution, but as yay is preferred by majority of users I will have to adopt third one 😖

@bartoszek
Copy link
Author

I've a canny plan, you can put a tail on i and call it a fix 😉

I'll just call prepare () from build(). Just made a small script checking prepare() for sed and if safe appending the call at the begining of build() with silent fail for already applied patches 😁
Beside few exceptions, seems to work like a charm.

@bartoszek
Copy link
Author

bartoszek commented Aug 24, 2019

Just wonder, if we could have yay export a variable when invoking makepkg to indicate to PKGBUILD that it is being run form within yay instance (YAY=1) 🤔 I'm have a few packages that checks if TRAVIS=1 to detect when they are run in Travis CI instance and adjust some compiler options accordingly (e.g. blender-2.7 drops cuda sm<5.0 to prevent oom kill)..

@bartoszek bartoszek reopened this Aug 24, 2019
@Morganamilo
Copy link
Contributor

Just wonder, if we could have yay export a variable when invoking makepkg to indicate to PKGBUILD that it is being run form within yay

No, if just for the fact that AUR helpers are unsupported and you don't need to care about them.

When it comes to this issue my suggestion to set the variables in every function that needs them is because I believe it is the correct way to write a PKGBUILD. Makepkg has flags for skipping functions, --noextract which skips prepare and --repackage which skips prepare and build. Both of which I've found useful and would break with your package.

When it comes to #1004 though, I stand by my opinion. Yay does not support dynamically changing depends. If yay users complain then tough luck, use makepkg or a helper that does support it.

Also I just read your pkgbuild. Turns out all your prepare function did was set variables. The point of the prepare function prepare the sources for building (hence why --noextract also skips prepare). It all belonged in build in the first place.

bartoszek pushed a commit to bartoszek/yay that referenced this issue Aug 24, 2019
Indicate to PKGBUILD that it is executed within yay context.

refers to Jguer#1006 Jguer#1004 Jguer#893
@bartoszek
Copy link
Author

all your prepare function did was set variables

Yap, that's why I've changed it.

Still thinks adding YAY=1 would be nice for PKGBUILD maintainers - just in case something more went sideways...

@eli-schwartz
Copy link

@bartoszek, allow me to put on my "I am an upstream makepkg developer" hat.

DO NOT USE PREPARE JUST TO SET VARIABLES.

You are violating the PKGBUILD specification, and as a result your PKGBUILD sucks.

Furthermore, makepkg --nobuild && makepkg --noextract exists for, well, precisely the purpose that it is a supported configuration, and we encourage people to use it when and where it makes sense. If we didn't want people to use it then we would not have such an option.

You may consider this to be my advice that the recommended way to write a PKGBUILD is to declare your variables in every function that needs them, or alternatively in the global scope. Not that it matters for your use case as it is very clearly information specific to the build() function and the invocation of cmake, which you are illegitimately moving elsewhere.

@eli-schwartz
Copy link

I don't even understand what your "magma" PKGBUILD is supposed to be doing, since apparently it passes different options to CMake depending on whether cuda is installed, then compiles with nvcc which I believe means it links to the cuda runtime libraries.

Therefore it would seem to me that the package must also add a depends+=('cuda').

You should be using something like _USE_CUDA=0 at the top of the PKGBUILD, and expect users to modify the variable if they want cuda support, and then using the variable to both pass additional CMake options and add the missing make/depends. Not somehow automagically producing different software just because the build environment happened to have cuda installed.

The result is of course that aurutils is unable to build the software using cuda. So much for using aurutils? Or maybe being unable to use cuda if you are an aurutils user is a feature?

More generally, what exactly do you feel is dirty about setting a global variable outside of the prepare function? The variable you already had is global too, as soon as you execute prepare() it adds a global variable. Luckily it is also defined with a leading underscore to prevent clashes...

If you wanted to avoid global variables you would need to use the local keyword, but of course that would mean it would not be available in build() anymore, since that is a different function...

If it is about checking for the existence of some directory, but only after prepare() started, then given that cuda is not listed as a dependency neither that directory nor anything else cuda related will ever be missing as a global check, but suddenly be present during prepare(). So you might as well have it be global. It is true that things like _pyver=$(expac -Q "%v" python) won't work if you are relying on those actually being dependencies, actually being synced before prepare(), and successfully running within the context of prepare() -- or better yet, build(). But in that case you just duplicate it everywhere it is needed...

@bartoszek
Copy link
Author

bartoszek commented Aug 25, 2019

You should be using something like _USE_CUDA=0 at the top of the PKGBUILD, and expect users to modify the variable if they want cuda support, and then using the variable to both pass additional CMake options and add the missing make/depends.

This won't reliably work across different AUR-helpers. Yay never executes makepkg --sync therefore no makedepends logic is possible (work this through with blender-2.7 package) therefor I've regress to PKGBUILD altering build logic base on packages user have installed, with an option to change the behavior with a variable.

@bartoszek
Copy link
Author

bartoszek commented Aug 25, 2019

More generally, what exactly do you feel is dirty about setting a global variable outside of the prepare function?

Nothing, just having some problems with variables using subshell to initialize (like mentioned _pyver) and I hastily decided to move all variables to prepare() without considering side effects. Thanks for pointing me to the right direction.
@eli-schwartz If I may ask an extra question, do you consider build system invocation (cmake) belong in prepare(), or build()?

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

No branches or pull requests

4 participants