[WIP] Chromium OS fork's Portage Ebuild changes

[Kangie]

Hi all,

I've been working on bringing the Chromium OS fork's Portage Ebuild changes to base shellcheck. It seems to be working pretty well and I was wondering if there's any interest in merging portage support.

I have a few questions:

1. What changes would need to be made to this patch to get it into shape to merge and maintain?
2. Are there any open issues that would block merging this (possibly finer grained control over shell dialect features #1341)?

It's been a while since I worked in Haskell, but I'm willing to give it a try if there's interest. I would like to be able to use this to validate the Gentoo ebuild repository!

I will need to redo the rebase by cherry picking individual commits to do proper attribution. I also plan to redo the chromium-overlay-sourced python scripts so that they can be consistently licensed.

Please review this work-in-progress PR and let me know your thoughts.

Thanks for your time!

[koalaman]

Sorry, it's been a busy year. Can you give me some background for this? Why does Gentoo's build system need build time variables in ShellCheck?

[Kangie]

No worries!

The short version is that Gentoo's package manager, Portage, uses ebuilds which are very bash-like.

Google's ChromeOS (and its open-source flavour ChromiumOS) use Portage to maintain... some parts of the system; I've never had to dig too far into it. At some point a ChromeOS or ChromiumOS developer added functionality to their fork of shellcheck so that it could be used to lint ebuilds without an excessive amount of false positives. The fork is outdated and for fun (?) I decided to update the patches to run on a more modern version of shellcheck.

I'd like to use it as part of my Gentoo ebuild QA, and several developers and members of the community seem interested as well. The ChromiumOS code seems to work well enough so I thought I'd see about getting those changes merged rather than maintaining another set of patches.

I guess my questions at this point are:

1. Would you be interested in merging the ebuild functionality?
2. What changes would you like to see so that this code is maintainable and sane?
3. Are there any open issues that would prevent this from proceeding?

Please note that I don't intend to suggest that this PR be merged in its current state; the PR is just a monolithic patch submitted for feedback with all of the ChromiumOS fork's commits rebased, modified, and squashed.

Cheers!

[hololeap]

The main problem that the patch seems to be solving is that shellcheck catches false positives on well-defined constants in .ebuild files:

$ shellcheck persistent-2.14.5.0.ebuild 

In persistent-2.14.5.0.ebuild line 1:
# Copyright 1999-2023 Gentoo Authors
^-- SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.


In persistent-2.14.5.0.ebuild line 4:
EAPI=8
^--^ SC2034 (warning): EAPI appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 9:
CABAL_FEATURES="lib profile haddock hoogle hscolour test-suite"
^------------^ SC2034 (warning): CABAL_FEATURES appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 12:
DESCRIPTION="Type-safe, multi-backend data serialization"
^---------^ SC2034 (warning): DESCRIPTION appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 13:
HOMEPAGE="https://www.yesodweb.com/book/persistent"
^------^ SC2034 (warning): HOMEPAGE appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 15:
LICENSE="MIT"
^-----^ SC2034 (warning): LICENSE appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 16:
SLOT="0/${PV}"
^--^ SC2034 (warning): SLOT appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 17:
KEYWORDS="~amd64 ~x86"
^------^ SC2034 (warning): KEYWORDS appears unused. Verify use (or export if used externally).


In persistent-2.14.5.0.ebuild line 43:
DEPEND="${RDEPEND}
^----^ SC2034 (warning): DEPEND appears unused. Verify use (or export if used externally).

For more information:
  https://www.shellcheck.net/wiki/SC2148 -- Tips depend on target shell and y...
  https://www.shellcheck.net/wiki/SC2034 -- CABAL_FEATURES appears unused. Ve...

[koalaman]

Is the set of variables known in advanced? How far would one get by adding .ebuild as a synonym for .bash and a hard coded list of known variables?

[Kangie]

Hi @koalaman,

While ebuild/eclass variables do change over time, it is possible to extract them automatically (which is how src/ShellCheck/PortageAutoInternalVariables.hs is constructed).

As best I can what you've described is basically what we're doing with this patch; set ebuilds to be an alias for bash script, hardcode .ebuild variables (which do not change often), add some special case handling for 'is this eclass inherited' so that none of the ENVVARS there interfere with standard bash parsing, and generate a list of eclass variables.

[koalaman]

My understanding was that it essentially adds a de facto build time dependency on Python and out-of-tree ChromeOS build files, and is then tailored to work on ChromeOS ebuilds. Is that not the case?

[Kangie]

adds a de facto build time dependency on Python and out-of-tree ChromeOS build files

I don't intend for the python scripts to remain in their current state - They're ripped directly from the ChromeOS/ChromiumOS ebuild repository and have a different licence from the rest of your code. It'll be easy enough to rework those down the track, if required, and probably even port them to Haskell. They were included in the WIP for completeness, and also because if you're not interested in merging a revised version of this PR then there's no point in doing that porting work!

It may also make more sense for these scripts to live in the Gentoo ebuild repository (associated with the shellcheck package) so that package maintainers / Gentoo users can use them to assist with providing updates upstream without the scripts being checked into VCS here.

It does add a dependency on the Gentoo ebuild repository (which is just a git repo that contains text files). This could be handled in a few ways:

• as part of the (manual) packaging process for a release the automated variable list could be updated by scripts
• Gentoo users/packagers can assist with maintenance of the eclass and ebuild-specific variables via PRs (etc), handle some of the 'eclass variable' stuff downstream between releases, and accept that if we miss getting updates to you by the time you decide to release an update then that's what gets packaged.
• Via CI/CD automation, with obvious benefits and disadvantages to trusting automation.

is then tailored to work on ChromeOS ebuilds

The intent here is to build against the Gentoo ebuild repository. Almost everything downstream of Gentoo will be able to take advantage of the changes as-is - they tend to mirror Gentoo eclasses at the very least. Some projects (Likely ChromeOS) may wish to regenerate src/ShellCheck/PortageAutoInternalVariables.hs if they use eclasses that have diverged from Gentoo, however as Portage is a source-based package management tool generating and applying these patches will be trivial; At the very least downstream distributions won't have to maintain the framework to enable this as a separate fork, only update the eclass variable list to suit their distribution.

I'm interested in hearing your thoughts. Thanks for your time this morning!

[koalaman]

I'm not opposed to merging a polished version of the functionality, the source and upkeep of the variable data is my only real concern. Could/should these be collected at runtime instead?

I imagine a significant motivator for the current approach was to simplify the patch and its rebasing -- which it definitely does -- rather than any benefits of collecting the data at compile time.

[Kangie]

Sorry, got hit by a migraine then swamped with work!

I'll see if I can come up with a performant approach that will gather the vast majority of the variables from eclasses at runtime - basically everything that goes into src/ShellCheck/PortageAutoInternalVariables.hs. It'll probably be a few weeks to a month or so before I have a change to push new code; I'll ping you here when I think it's looking good.

[koalaman]

Sounds good! Don't worry too much about threading it through into the right places, I can help out with that. The main thing is a function in IO that takes an ebuild directory and parses out the necessary information.

[Kangie]

Quick update, it's not forgotten I just moved halfway across a continent and hurt both of my arms.

@hololeap has made some good progress on getting us a runtime ebuild repository parser that (if I'm remembering our discussions properly) should be a lot more flexible when working with non-gentoo repositories.

I'll try and get back into this "soon", recovery permitting :)

[hololeap]

@koalaman I created a function that scans the system and creates an IO (Map String [String]), where String is the name of the eclass and [String] is the list of variables that are inherited from it. The Map should be able to plug in here (from the ChromiumOS code):

shellcheck/src/ShellCheck/Data.hs

Line 156 in 22bd5a4

portageAutoInternalVariables

The problem is that I don't see anywhere to run in the IO monad. The closest entry point is all the way in the main shellcheck.hs:

shellcheck/shellcheck.hs

Line 247 in dd747b2

result <- checkScript sys checkspec

Ideally, the loaded file needs to be determined to be an ebuild before attempting to scan for eclasses, which could also fail on non-Gentoo systems. If found, the eclass data would then need to be passed deeper into the code, and I don't see an existing way to do that.

[hololeap]

I put my current code up here. The most relevant parts are

https://github.com/hololeap/shellcheck/blob/08ae7ef83621ece82d6644b95d95abdde62ee696/src/ShellCheck/PortageVariables.hs#L40-L55

[hololeap]

272ef81 is my attempt to pass the scanned IO (Map EclassName [EclassVar]) into Parameters so that they can be read via checkScript.

dfa920c adds a dependency on attoparsec and text. attoparsec seems to perform roughly 2x better than parsec.

[Kangie]

I will test these changes when I'm home from dog show stuff, thanks for committing them here.

Runs portageq to determine repository names and locations. Emits a warning if an IOException is caught when attempting to run portageq.

Can we avoid the external dependency by reading up on the Gentoo PMS?. It's probably sane to assume that Gentoo / Portage users will have access to portageq but I do wonder if that holds true for Paludis and Pkgcore.

Regardless it looks great and I can't wait to test it soon

[koalaman]

I haven't had a chance to look deeply into your changes yet, but here are my thoughts for plumbing:

0138a6f

This adds a function siGetPortageVariables to ShellCheck's SystemInterface, which is used for system access type functions like "read file by name". The data can now be fetched on demand, and the core functionality can be robustly tested on non-Portage systems.

Some thoughts:

• It would be fairly simple to look for (static) inherit commands and pick the relevant keys from the map based on that. Is that the plan? Do eclass files inherit from each other so that this would need to be done recursively?
• I'm not familiar with how portage repos are structured. Would one ideally include a class name so that siGetPortageVariables "foo" could only look at foo.eclass or whatever, instead of parsing all files (which I imagine was mostly done in the proof-of-concept because it happened at compile time).
• A final version probably won't need to depend on attoparsec. The current parser is likely slow due to unintentionally excessive lookaheads like in takeLine = manyTill anyChar (try endOfLine) (compared to takeLine = many $ noneOf "\n"). If it just reads line by line, then it may not warrant a monadic parser at all. This can all be sorted out later when things work. Same with improving robustness with regard to spaces and such.

[hololeap]

Thanks for the attention on this. I can give some insight on your questions.

---

It would be fairly simple to look for (static) inherit commands and pick the relevant keys from the map based on that. Is that the plan? Do eclass files inherit from each other so that this would need to be done recursively?

The ChromiumOS code has some kind of functionality in place to check for inherit commands:

shellcheck/src/ShellCheck/Analytics.hs

Lines 2094 to 2099 in e3d8483

	getInheritedEclasses :: Token -> [String]
	getInheritedEclasses root = execWriter $ doAnalysis findInheritedEclasses root
	where
	findInheritedEclasses cmd
	| cmd `isCommand` "inherit" = tell $ catMaybes $ getLiteralString <$> (arguments cmd)
	findInheritedEclasses _ = return ()

eclass files do inherit from each other, so it would make sense to do this recursively

---

I'm not familiar with how portage repos are structured. Would one ideally include a class name so that siGetPortageVariables "foo" could only look at foo.eclass or whatever, instead of parsing all files (which I imagine was mostly done in the proof-of-concept because it happened at compile time).

The eclasses are stored in a eclass/ directory at the root of the repo. It isn't possible to tell which repo holds a given eclass based on the name, but the repos could be enumerated in the order of their priority, in order to look for a .eclass which has been inherited. The list of available repos on the system is quickly found using a portage-specific command:

shellcheck/src/ShellCheck/PortageVariables.hs

Lines 45 to 55 in 08ae7ef

	-- | Run @portageq@ to gather a list of repo names and paths, then scan each
	-- one for eclasses and ultimately eclass metadata.
	scanRepos :: IO [Repository]
	scanRepos = do
	let cmd = "/usr/bin/portageq"
	let args = ["repos_config", "/"]
	out <- runOrDie cmd args
	case parse reposParser "scanRepos" out of
	Left pe -> fail $ show pe
	Right nps -> do
	forM nps $ \(n,p) -> Repository n p <$> getEclasses p

Parsing all .eclass files is not necessary but was indeed intended to produce a functioning proof-of-concept.

---

A final version probably won't need to depend on attoparsec. The current parser is likely slow due to unintentionally excessive lookaheads like in takeLine = manyTill anyChar (try endOfLine) (compared to takeLine = many $ noneOf "\n"). If it just reads line by line, then it may not warrant a monadic parser at all. This can all be sorted out later when things work. Same with improving robustness with regard to spaces and such.

I admittedly am not very familiar with optimizing parsec for speed, and I felt that attoparsec would be a good fit here because

1. It is optimized for doing bulk parses where you don't need easy-to-read error messages containing location state
2. It has low-level functions such as takeWhile, which I cannot find equivalents of in parsec.

I understand that adding another dependency isn't ideal either, and that with the other optimizations (such as only scanning the needed .eclass files) parsec may be more than adequate.

[koalaman]

Thanks! I managed to get a Gentoo environment running in Docker, and got a bit further on this in https://github.com/koalaman/shellcheck/tree/ebuild

It now runs end-to-end, but I ended up reverting a number of changes when going from Parameters to SystemInterface, and I'm still working on adding them back one by one.

[koalaman]

It's been a busy September, but https://github.com/koalaman/shellcheck/tree/ebuild now has most of the changes from the original patch, and I replaced the lovely attoparsec code with some janky regex matching and regular IO to avoid pulling in the additional dependencies.

In the original PR, checking a small ebuild took 240ms, of which 200ms was invoking portageq. With this uglier approach it takes 310ms, but I think this is fine since it only happens when checking ebuild files, and only once per shellcheck invocation. This is all on a default Gentoo system though, I don't know how repos tend to scale.

The original PR had a check for KEYWORDS in 9999.ebuild files, but it was gated on inheriting an eclass cros-workon, which I'm sure is useful but it seemed a bit too niche to support upstream (it could be implemented in a site specific Custom.hs by looking up the filename in tokenPositions in the Parameters).

Could you try it out and let me know how it works in practice, and what I may have missed from the original PR?

[Kangie]

I replaced the lovely attoparsec code with some janky regex matching and regular IO to avoid pulling in the additional dependencies.

It's your software!

In the original PR, checking a small ebuild took 240ms, of which 200ms was invoking portageq. With this uglier approach it takes 310ms, but I think this is fine since it only happens when checking ebuild files, and only once per shellcheck invocation. This is all on a default Gentoo system though, I don't know how repos tend to scale.

I'll get back to you, but ::gentoo is probably the largest repository out there - I have about 10 enabled but they're orders of magnitude smaller than ::gentoo,

The original PR had a check for KEYWORDS in 9999.ebuild files, but it was gated on inheriting an eclass cros-workon, which I'm sure is useful but it seemed a bit too niche to support upstream (it could be implemented in a site specific Custom.hs by looking up the filename in tokenPositions in the Parameters).

That seems very ChromiumOS specific. I wouldn't worry about it too much - 9999 is a convention that means 'from VCS sources' and the eclass in question seems to be a helper for ChromiumOS to build things entirely from git sources.

Could you try it out and let me know how it works in practice, and what I may have missed from the original PR?

I'll build a copy right now!

[Kangie]

It took ages to update my hackage index. I've got to run off to work shortly so detailed testing will have to wait for a bit but overall it looks great.

It's probably worth suppressing the 'eclass dir does not exist' message outside of verbose/debug - most repos just use gentoo eclasses, though there are a good number that do ship their own eclasses.

Unable to find .eclass files: /var/db/repos/crossdev/eclass: getDirectoryContents:openDirStream: does not exist (No such file or directory)
Unable to find .eclass files: /var/db/repos/kangie/eclass: getDirectoryContents:openDirStream: does not exist (No such file or directory)
Unable to find .eclass files: /var/db/repos/kubler/eclass: getDirectoryContents:openDirStream: does not exist (No such file or directory)
Unable to find .eclass files: /var/db/repos/steam-overlay/eclass: getDirectoryContents:openDirStream: does not exist (No such file or directory)

Otherwise, when run across an ebuild that I maintain the branch only returned valid warnings - ebuild syntax isn't flagged as a warning.

I'll try and find some actual issues but that's a positive result so far!

[Kangie]

I've got a Curl ebuild bump to prepare for, which gave me a good opportunity to do some testing.

For net-misc/curl/curl-9999.ebuild:

• Whitelist ${PN} for SC2206 (portage won't provide something that can be split) (Edit: ${P}, ${PV}, ${V} too now that I think about it; probably also SC2086)
• Whitelist ${ED} for SC2115 (portage will always ensure that $ED is not empty)
• MULTILIB_* here comes from multilib-build.eclass - that's inherited by multilib-minimal, which is inherited by this ebuild.

In curl-9999.ebuild line 107:
MULTILIB_WRAPPED_HEADERS=(
^----------------------^ SC2034 (warning): MULTILIB_WRAPPED_HEADERS appears unused. Verify use (or export if used externally).

In curl-9999.ebuild line 111:
MULTILIB_CHOST_TOOLS=(
^------------------^ SC2034 (warning): MULTILIB_CHOST_TOOLS appears unused. Verify use (or export if used externally).

This guy is consumed by portage! Can we whitelist it?

https://github.com/gentoo/portage/blob/18db7f8cc8c750b50100680dcf288f3177173669/bin/install-qa-check.d/90config-impl-decl#L36

In curl-9999.ebuild line 115:
QA_CONFIG_IMPL_DECL_SKIP=(
^----------------------^ SC2034 (warning): QA_CONFIG_IMPL_DECL_SKIP appears unused. Verify use (or export if used externally).

Edit: We can probably scan over /usr/lib/portage/${PYTHON_IMPL}/install-qa-check.d - I doubt it'll hurt to just scan everything under /usr/lib/portage/**/install-qa-check.d for stray variables. I don't know how that will impact non-portage ebuild package managers though! It's probably worth suppressing a 'not found' outside of debug/verbose mode to cover that use case.

I'll try and come up with a better way to find edge cases like this but it looks good so far!

[Kangie]

mycmakeargs is consumed by the cmake eclass:

In createrepo_c/createrepo_c-1.0.0.ebuild line 44:
        local mycmakeargs=(
              ^---------^ SC2034 (warning): mycmakeargs appears unused. Verify use (or export if used externally).

emesonargs is consumed by the meson eclass (this is a better example, we definitely call meson_src_configure:

In zchunk/zchunk-1.3.1.ebuild line 32:
        local emesonargs=(
              ^--------^ SC2034 (warning): emesonargs appears unused. Verify use (or export if used externally).

These are pretty minor nits that could be worked around with documentation IMO, but there are a whole class of false-positives that might be easily addressable with some haskell trickery - overall everything I've checked so far has so much less noise than it did proviously.

[koalaman]

I see. So, I guess there are two surmountable issues:

• Support for multiple levels of inheritance
• Support for @VARIABLE and maybe @USER_VARIABLE in addition to @ECLASS_VARIABLE

And some significantly more annoying issues:

• There are a number of variables with very generic names like P and V that ebuild checks want to have special handling for
• There are special cases (bugs?) for declared variables where it says # @VARIABLE: MYCMAKEARGS but it actually looks for mycmakeargs
• There is a (presumably evolving) long tail of variables like QA_CONFIG_IMPL_DECL_SKIP that are not declared or listed anywhere

I'm starting to wonder about the relative value of supporting and maintaining ebuild support with workarounds upstream, compared to some more flexible 80% solution like having CI awk/grep the variables into assignments file that shellcheck can read with # shellcheck source=myfile on a source /dev/null.

[thesamesam]

And some significantly more annoying issues:

* There are a number of variables with very generic names like `P` and `V` that ebuild checks want to have special handling for

Yeah, I can see how this part in particular is a nuisance. There's not that many of them and they're well-defined at least though.

* There are special cases (bugs?) for declared variables where it says `# @VARIABLE: MYCMAKEARGS` but it actually looks for `mycmakeargs`

This one is a quirk because the user might set MYCMAKEARGS in the environment, while mycmakeargs is to be used within ebuilds. Suppressing warnings/errors for @USER_VARIABLE would handle that.

* There is a (presumably evolving) long tail of variables like `QA_CONFIG_IMPL_DECL_SKIP` that are not declared or listed anywhere

We could probably just ignore QA_*. They're documented in ebuild(5) but that's all.

I'm starting to wonder about the relative value of supporting and maintaining ebuild support with workarounds upstream, compared to some more flexible 80% solution like having CI awk/grep the variables into assignments file that shellcheck can read with # shellcheck source=myfile on a source /dev/null.

I've not been involved in the discussion thus far so I don't want to say "yes, definitely do that", but it sounds appealing and like a reasonable fit given all the quirks.

[hololeap]

An observation: all of the changes in this PR are essentially adding things to whitelists for various checks (SC2034, SC2206, SC2115, etc.)

What if shellcheck looks for external whitelists based on the extension of the file (e.g. .ebuild)?

For instance, the whitelists could exist in a directory tree, the structure looking like:

$ find /usr/share/shellcheck/whitelists/
/usr/share/shellcheck/whitelists/ebuild/core.wl
/usr/share/shellcheck/whitelists/ebuild/eclasses/cmake.wl
/usr/share/shellcheck/whitelists/ebuild/eclasses/haskell-cabal.wl

The individual .wl (standing for "whitelist") files could look like:

# Comment

@SC2206
- PN
- P
- PV
- V

@SC2115
- ED

• If the extension is .ebuild and /usr/share/shellcheck/whitelists/ebuild/ doesn't exist, then just emit a warning
• Everything under /usr/share/shellcheck/whitelists/ebuild/ is either a .wl file or a directory containing .wl files (this only helps keeps things organized and prevents a giant monolithic .wl file)
• The whitelists would be maintained separately by Gentoo
  • The whitelists for eclasses could be generated by a script (similar to app-doc/eclass-manpages)
  • Packaged separately (e.g. dev-util/shellcheck-ebuild-whitelists)
• Whitelist files could be parsed using parsec, so it won't add any extra dependencies.