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

Add and Update foreman_supervisory_authority dependencies #11157

Conversation

ochnerd
Copy link

@ochnerd ochnerd commented Aug 22, 2024

Add base64-0.2.0 as dependency for llhttp-ffi
Update http to 5.2.0
Update http-form_data to 2.3.0
Add llhttp-ffi-0.5.0 as dependency for http

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

1 similar comment
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@dgoetz
Copy link
Member

dgoetz commented Aug 22, 2024

ok to test

@dgoetz
Copy link
Member

dgoetz commented Aug 22, 2024

I think you added the new packages manually instead of using the script, so they are missing from package_manifest.yaml and comps/*.

@ochnerd
Copy link
Author

ochnerd commented Aug 22, 2024

I used the script but my Mac kind of complaint about something. I check that and push again, thanks for testing!

@ochnerd
Copy link
Author

ochnerd commented Aug 22, 2024

@dgoetz , should ./add_gem_package.sh add them to package_manifest.yaml?

@dgoetz
Copy link
Member

dgoetz commented Aug 22, 2024

Yes, there is a function add_to_manifest which maps the repository to the corresponding section of the manifest and the functions add_gem_to_all_comps and add_gem_to_comps ensure packages are added to matching comps.
The manifest is used as base for obal (and all other tooling) resulting in the linting error.

Add rubygem-base64 package -> dependency for rubygem-llhttp-ffi
Add rubygem-llhttp-ffi package -> dependency for http
Bump rubygem-http to 5.2.0-1
Bump rubygem-http-form_data to 2.3.0-1
@ochnerd ochnerd force-pushed the rpm/develop_update_foreman_supervisory_authority_dependencies branch from 3a60896 to a132643 Compare August 22, 2024 14:20
@dgoetz
Copy link
Member

dgoetz commented Aug 22, 2024

Repoclosure now fails with

Error: Repoclosure ended with unresolved dependencies (3) across 1 packages.
package: rubygem-http-5.2.0-1.el8.noarch from repo0
  unresolved deps (3):
    (rubygem(base64) >= 0.1 with rubygem(base64) < 1)
    (rubygem(http-form_data) >= 2.2 with rubygem(http-form_data) < 3)
    (rubygem(llhttp-ffi) >= 0.5.0 with rubygem(llhttp-ffi) < 0.6)

So with base64 being 0.2.0, llhttp-ffi 0.5.0, and http-form_data 2.3.0, this is a problem of order. Not sure if you need to split up the PR once more or having the packages in single commits in correct order would be enough (this is what I typically do).

@ochnerd
Copy link
Author

ochnerd commented Aug 23, 2024

Ahh I see the problem. If I think about the order I will probably need to open multiple PR when I understand correctly the dependencies need to already be in the repository. So Single PRs it is. I would like to leave this PR Open, include the links to the new PRs and close it after the new PRs have been merged.

@ochnerd
Copy link
Author

ochnerd commented Aug 23, 2024

base64 PR #11164 has no dependencies
llhttp-ffi PR #11165 has dependencies which can be resolved
http-form_data PR #11166 has no dependencies

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think you may be right we need all dependencies satisfied before repoclosure can work. I thought we grouped them, but now that I take a closer look we don't.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't add base64. #11119 works at documenting how you should deal with it. I left other comments below with more specific examples.

Copy link
Author

Choose a reason for hiding this comment

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

Because of the dependency Issues I split this PR into the other 3 (I should close this PR i think):
#11165
#11166
#11167

Summary: HTTP should be easy
Group: Development/Languages
License: MIT
URL: https://github.com/httprb/http
Source0: https://rubygems.org/gems/%{gem_name}-%{version}.gem
Copy link
Member

Choose a reason for hiding this comment

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

Please prefer the default ruby gems when possible

Suggested change
Source0: https://rubygems.org/gems/%{gem_name}-%{version}.gem
Source0: https://rubygems.org/gems/%{gem_name}-%{version}.gem
Requires: (rubygem(base64) >= 0.1 or ruby-default-gems < 3.4)

%{?scl:scl enable %{scl} - << \EOF}
gem spec %{SOURCE0} -l --ruby > %{gem_name}.gemspec
%{?scl:EOF}
%setup -q -n %{gem_name}-%{version}
Copy link
Member

Choose a reason for hiding this comment

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

And then drop the base64 dependency here:

Suggested change
%setup -q -n %{gem_name}-%{version}
%setup -q -n %{gem_name}-%{version}
%gemspec_remove_dep -g base64 '~> 0.1'

%exclude %{gem_instdir}/.gitignore
%exclude %{gem_instdir}/.rubocop.yml
%exclude %{gem_instdir}/.travis.yml
%{gem_instdir}/.rubocop
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
%{gem_instdir}/.rubocop
%exclude %{gem_instdir}/.rubocop

Comment on lines +4 to +5
# The directory structure is very special and
# does not really align with any other package
Copy link
Member

Choose a reason for hiding this comment

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

I don't see what's so different. This looks fairly standard to me for a gem with native extensions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why it gets sorted very different. We try to set the locale for consistent sorting but perhaps you don't have that locale installed?

export LC_COLLATE=en_GB.UTF-8

@ochnerd
Copy link
Author

ochnerd commented Sep 16, 2024

Closed in favor for splitted PR
#11165
#11166
#11167

@ochnerd ochnerd closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants