search

vn971 / rua

Build tool for Arch Linux providing control, review and jailed build options
GNU General Public License v3.0
427 stars 41 forks source link

Respect makepkg config files in addition to environment variables #110

Closed VukoDrakkeinen closed 4 years ago

VukoDrakkeinen commented 4 years ago

Previously, if set only e.g. in /etc/makepkg.conf, options like PKGEXT were ignored and defaults were used instead.

Unfortunately, since makepkg configuration files are valid scripts, reading them involves shelling out. Non-ideal, but I feel that ignoring them is strictly worse.

vn971 commented 4 years ago

Besides the minor comments above, everything seems good. Thanks a lot for the PR! I'll also need to playtest it a bit, but afterwards, I expect to merge.

VukoDrakkeinen commented 4 years ago

Man, writing concise and clear error messages/warnings is HARD. I'm still not fully satisfied, but I think it should be somewhat fine now. What do you think?

vn971 commented 4 years ago

Merged! Thanks a lot! I did touch up the logging just a tiny bit: https://github.com/vn971/rua/commit/db9c6867fdc5dc82627cda6466196cb1448b5edb An extra pair of eyes is always useful in things like docs. Also, I removed the "$" from the logging if you don't mind, because this symbol might not necessarily be written by the user, and could possibly confuse (IDK? Just left it out for simplicity).

vn971 commented 4 years ago

UPD: hmm, we apparently didn't catch it and there were no tests for that, but rua now breaks if you execute a command like rua search abcd from root. It will complain saying:

rua search test
RUA does not allow building as root.
Also, makepkg will not allow you building as root anyway.

so basically, the users::get_current_uid() check is now done earlier/eagerly, and we construct RuaPaths even when we don't need it (as e.g. for search action).

I'll think on how to de-compose those two back.

vn971 commented 4 years ago

I think the problem can be solved by moving the makepkg config checks to the function that instantiates RuaPaths, because one only makes sense with the other, currently. I'll try to do that now.

VukoDrakkeinen commented 4 years ago

UPD: hmm, we apparently didn't catch it and there were no tests for that, but rua now breaks if you execute a command like rua search abcd from root.

Hmm, in a similar vein, non-building commands (search, info, etc) will fail when executed inside rua (Error: another RUA instance already running.)

vn971 commented 4 years ago

@VukoDrakkeinen yeah, indeed. I think that problem can be tackled separately though, it shouldn't be hard.

Meanwhile I'm finishing the fix to make root and PKGEXT evaluations "lazy" (only for needed commands).