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

Support sub-packages having arch=('all') v3 #262

Open
wants to merge 2 commits into
base: alpha
Choose a base branch
from

Conversation

bithium
Copy link
Contributor

@bithium bithium commented Apr 11, 2023

This patch allows sub-packages to override the arch attribute setting it to all.

The use case for this feature is when generating documentation packages that can be installed in any architecture.

@bithium
Copy link
Contributor Author

bithium commented Apr 11, 2023

I have added the changes you requested (I think).

I created a new PR because my other PR as associated to a fork from Jabir-Srj/makedeb, for whatever reason.

@hwittenborn
Copy link
Member

Hey I'm back, terribly sorry for the wait! Some other projects I'm working on have been taking up a ton of my time, and I'm just now getting back to being able to work on makedeb a bit.

That Jabir-Srj/makedeb thingy is probably from an issue I had when restoring the GitHub repository (I accidentally deleted this repo from a bug with the GitHub CLI, it's a long story :P), and it did a bunch of janky stuff that associated this repository with that one for some reason. Everything got cleaned up a bit ago though, and it should all be good now.

I'll go ahead and get your PR reviewed real quick :)

test/tests/arch.bats Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be good to make a test that makes sure that if a global arch=('all') is set, that arch=('amd64') or anything inside of a package_* function fails.

I can get this done myself before a merge if need be, I'm just making a note of it here so I know it needs to get done.

Copy link
Member

Choose a reason for hiding this comment

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

Or should that be desired behavior? I hadn't really thought about subpackages straight-up having different architectures like i386 and stuff like that. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should that be desired behavior? I hadn't really thought about subpackages straight-up having different architectures like i386 and stuff like that. What do you think?

I don't think that sub-packages should have arbitrary architectures.

To me the only use case that make sense is a top-level arch=(amd64|i386|...) and sub-packages that are arch independent, for documentation and/or assets files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably be good to make a test that makes sure that if a global arch=('all') is set, that arch=('amd64') or anything inside of a package_* function fails.

I can get this done myself before a merge if need be, I'm just making a note of it here so I know it needs to get done.

I am not sure how to go about checking this.

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be good to stop sub-packages from having specific architecture too, it can always be expanded in the future if need be as well.

I am not sure how to go about checking this.

You can see if the ${INFAKEROOT} variable is set, makedeb sets that when things like package() are being ran (see alpha:src/main.sh#L1340 for an example of the syntax that's used across the codebase).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I has thinking if this could be done at linting time, any pointers on how to do this ?

This patch allows sub-packages to override the arch attribute setting it
to `all`.

The use case for this feature is when generating documentation packages
that can be installed in any architecture.
@bithium bithium force-pushed the feature/support_all_arch_for_subpackages branch from b9a1470 to c2dffa6 Compare June 5, 2023 09:38
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

Successfully merging this pull request may close these issues.

None yet

2 participants