Skip to content

[RFC] [WIP] use bash arrays for some template fields#42656

Draft
classabbyamp wants to merge 4 commits into
void-linux:masterfrom
classabbyamp:declare-a
Draft

[RFC] [WIP] use bash arrays for some template fields#42656
classabbyamp wants to merge 4 commits into
void-linux:masterfrom
classabbyamp:declare-a

Conversation

@classabbyamp
Copy link
Copy Markdown
Member

@classabbyamp classabbyamp commented Mar 8, 2023

this will allow select variables to be defined as arrays or strings. if a string is specified, it will be converted to an array for internal usage. if it is an array, it passes through untouched. this should allow for a smooth transition to using more arrays inside xbps-src, and should solve issues with shell quoting in things like configure_args, make_check_args, etc.

to start, I've only done this with configure_args, and fixed some templates

would like to get some feedback before continuing, as this will be a fairly massive undertaking. what fields would be valuable as arrays? (I'm thinking mostly the *_args variables) should the internal string -> array conversion happen or should we just go all-in?

Testing the changes

  • I tested the changes in this PR: YES|briefly|NO

[ci skip]

@classabbyamp classabbyamp added WIP Work in progress xbps-src xbps-src related labels Mar 8, 2023
@classabbyamp classabbyamp requested a review from a team March 8, 2023 09:48
@tranzystorekk
Copy link
Copy Markdown
Contributor

tranzystorekk commented Mar 8, 2023

*depends variables also seem like a good pick for this.

I haven't checked if there's any convenient mechanism for concatenating arrays, which would be nice in python packages as an alternative to checkdepends="${depends} ...".

If possible, it would be nice if we could make this optional, i.e. still parse variables the old way, but if we detect an array, we skip the conversion step and just take it as-is.

EDIT: sorry I skimmed through the part where you already say it's optional by design.

@classabbyamp
Copy link
Copy Markdown
Member Author

classabbyamp commented Mar 8, 2023

I haven't checked if there's any convenient mechanism for concatenating arrays

for the same array, just foo+=(new_element), for different:

$ foo=(foo bar baz)
$ bar=("${foo[@]}" idk)
$ declare -p foo
declare -a foo=([0]="foo" [1]="bar" [2]="baz")
$ declare -p bar
declare -a bar=([0]="foo" [1]="bar" [2]="baz" [3]="idk")

@tranzystorekk
Copy link
Copy Markdown
Contributor

I haven't checked if there's any convenient mechanism for concatenating arrays

for the same array, just foo+=(new_element), for different:

$ foo=(foo bar baz)
$ bar=("${foo[@]}" idk)
$ declare -p foo
declare -a foo=([0]="foo" [1]="bar" [2]="baz")
$ declare -p bar
declare -a bar=([0]="foo" [1]="bar" [2]="baz" [3]="idk")

Hm, for me the added explicitness of "${foo[@]}" is a plus, but probably not everyone would agree.

Copy link
Copy Markdown
Member

@ahesford ahesford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of all the xbps-src improvement proposals, this is the one I most strongly support. Bash arrays will solve a lot of problems in a lot of templates and we should move any variable currently space-separated to an array for proper handling. This includes:

  • *depends
  • *args
  • distfiles
  • checksum
  • nostrip_files
  • ignore_elf*
  • make_dirs (either one value per entry to simplify iteration, or just split every component so that space handling in paths would work as expected... not sure yet)
  • subpackages
  • nopie_files
  • shlib_{provides,requires}
  • reverts
  • alternatives
  • font_dirs
  • dkms_modules
  • register_shell
  • tags
  • perl_configure_dirs
  • archs
  • conf_files
  • mutable_files
  • skip_extraction
  • make_check_pre
  • make_*_target
  • build_helper
  • make_cmd
  • configure_script
  • conflicts
  • replaces

We can do this by attrition, but relatively quickly, with a linter that fails if any of the should-be-arrays is a string that will split into an array with more than one element. Thus, the only time we accept the current syntax in future template updates is as a shorthand for a one-element array.

Comment thread common/xbps-src/shutils/common.sh Outdated
@paper42
Copy link
Copy Markdown
Contributor

paper42 commented Mar 8, 2023

this might be for another issue/PR, but we might also want to consider using associative arrays for build_options like this:

declare -A buildopts=([docs]=0 [wayland]=1 [xorg]=1)

if this was declared before buildopts are used (= usually before configure_args), we could parse the template only once instead of twice if we also used functions for accessing build options instead of variables like currently (because buildopts has to be combined with options from cli args and the config). The only disadvantage I can think of is that it would be ugly to have declare -A right at the top of the template.

@classabbyamp
Copy link
Copy Markdown
Member Author

I think that should be out of scope for this PR, but it may be useful to do

@classabbyamp
Copy link
Copy Markdown
Member Author

We can do this by attrition, but relatively quickly, with a linter that fails if any of the should-be-arrays is a string that will split into an array with more than one element.

I like this, and I've done this in the array conversion function directly, instead of e.g. changing xlint. I feel that using xlint for that would just be a mess

Comment thread common/xbps-src/shutils/common.sh Outdated
Comment thread common/xbps-src/shutils/common.sh Outdated
@leahneukirchen
Copy link
Copy Markdown
Member

I'm not a big fan of these 0,1,2 parameter... could we use str ary or something?

@leahneukirchen
Copy link
Copy Markdown
Member

or 1 and n ;)

@classabbyamp
Copy link
Copy Markdown
Member Author

i think licenses could be an array too, but we'd need to keep spdx expressions together for sanity, like licenses=(LGPL-2.1-or-later "BSD-3-Clause WITH BullshitException")

@Duncaen
Copy link
Copy Markdown
Member

Duncaen commented Mar 9, 2023

We have nothing really to gain from making licenses an array, at the moment its free text that is not interpreted by anything, making it a shell array now might cause conflicts if we would want to actually use SPDX expressions where you can have nesting and AND/OR operators (MIT AND (LGPL-2.1-or-later OR BSD-3-Clause)).

https://spdx.github.io/spdx-spec/v2-draft/SPDX-license-expressions/

@classabbyamp
Copy link
Copy Markdown
Member Author

classabbyamp commented Mar 9, 2023

I'm not a big fan of these 0,1,2 parameter... could we use str ary or something?

i've made it a bit more readable, but I suspect the code will change more once more things are arrays

@github-actions github-actions Bot added the Stale label Jun 8, 2023
@classabbyamp classabbyamp removed the Stale label Jun 8, 2023
@void-linux void-linux deleted a comment from github-actions Bot Jun 8, 2023
@classabbyamp classabbyamp mentioned this pull request Jul 11, 2023
@github-actions github-actions Bot added the Stale label Sep 7, 2023
@classabbyamp classabbyamp removed the Stale label Sep 7, 2023
@void-linux void-linux deleted a comment from github-actions Bot Sep 7, 2023
@classabbyamp classabbyamp force-pushed the declare-a branch 2 times, most recently from 7e0ab2b to b5a2bff Compare November 2, 2023 15:54
@classabbyamp
Copy link
Copy Markdown
Member Author

I think the best way to go about this will be:

  1. don't try to convert strings to arrrays
  2. don't try to assume strings are single-element arrays
  3. have some utility (maybe written in go with shfmt's library) that can do a best-effort conversion of templates

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 1, 2024

Pull Requests become stale 90 days after last activity and are closed 14 days after that. If this pull request is still relevant bump it or assign it.

@github-actions github-actions Bot added the Stale label Feb 1, 2024
@classabbyamp classabbyamp self-assigned this Feb 1, 2024
@classabbyamp classabbyamp removed the Stale label Feb 1, 2024
`ensure_array` will ensure the named variables are arrays if defined.
This allows us to not need to convert all packages to arrays
immediately, as they can be migrated the next time someone builds them.

`ensure_array_subpkg` does the same thing as `ensure_array`, but is also
passed the subpkg for better error message context.

`array_contains` is a helper function to make checking if an array
contains a value easier.
@classabbyamp classabbyamp marked this pull request as draft June 13, 2025 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP Work in progress xbps-src xbps-src related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants