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

fix(fedora): Recent syntax changes with DNF5 #907

Merged
merged 6 commits into from
Nov 11, 2024

Conversation

Angxddeep
Copy link
Contributor

@Angxddeep Angxddeep commented Nov 3, 2024

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

There have been a recent syntax change in dnf5 which comes preinstalled with every distro based on fedora 41 or later, therefore I have updated all the setup scripts which requires --add-repo and replaced it with addrepo --from-repofile="https://...". Also @jeevithakannan2 has added a version check so it doesn't conflict with systems that are not on dnf5.

Testing

Works. Although there is an issue with upstream braves repo file, I have created an issue on their GitHub page. It will start working again once they update the upstream repo file. I think this should be merged regardless as currently no script is working with dnf5 after this 3 out of 4 will start working and after brave fixes the issue all 4 will work.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@Angxddeep Angxddeep marked this pull request as draft November 3, 2024 00:56
@Angxddeep Angxddeep marked this pull request as ready for review November 3, 2024 01:01
@jeevithakannan2
Copy link
Contributor

I think adding a version check is more robust way

@Angxddeep
Copy link
Contributor Author

I think adding a version check is more robust way

Do we really need to do that, most fedora downstream will switch to dnf5 if they haven't already done it.

@jeevithakannan2
Copy link
Contributor

Are they switching to dnf5 in fedora 39 and 40 also?

@adamperkowski adamperkowski added the bug Something isn't working label Nov 4, 2024
@adamperkowski adamperkowski marked this pull request as draft November 4, 2024 01:40
@adamperkowski
Copy link
Collaborator

Are they switching to dnf5 in fedora 39 and 40 also?

No idea. @Angxddeep please research this, add a version check (if applicable) and then mark this PR as ready for review. Thanks.

@lj3954
Copy link
Contributor

lj3954 commented Nov 4, 2024

Are they switching to dnf5 in fedora 39 and 40 also?

No. This PR absolutely cannot be merged as is.

@Angxddeep
Copy link
Contributor Author

Angxddeep commented Nov 4, 2024

I am a bit busy with life for a week. Ill will try to do it after that

@Angxddeep Angxddeep marked this pull request as ready for review November 6, 2024 05:38
@jeevithakannan2
Copy link
Contributor

jeevithakannan2 commented Nov 6, 2024

fedora 39, 40 have dnf5 package. So 64cb3c8 this might not work

@Angxddeep
Copy link
Contributor Author

Angxddeep commented Nov 6, 2024

fedora 39, 40 have dnf5 package. So 64cb3c8 this might not work

They only have dnf5 as experimental in the repos, it's not installed by default

@lj3954
Copy link
Contributor

lj3954 commented Nov 6, 2024

Yes, but it still could be installed; the dnf symlink will continue to point to dnf4 in that case. You would need to explicitly call dnf5 to ensure the correct program is used.

@jeevithakannan2
Copy link
Contributor

jeevithakannan2 commented Nov 6, 2024

What if the user symlinked the dnf to dnf5?

dnf --version and some cuts this reduces complexity

@Angxddeep
Copy link
Contributor Author

Yes, but it still could be installed; the dnf symlink will continue to point to dnf4 in that case. You would need to explicitly call dnf5 to ensure the correct program is used.

I am going to be honest, fedora 39 will be EOL in a month and linutil isn't popular enough and most people will upgrade to 41 in no time. As is said I am a bit busy I think we should merge this so at least it works for people who have upgraded to F41. If you guys can submit changes that will be good too

core/tabs/applications-setup/browsers/brave.sh Outdated Show resolved Hide resolved
core/tabs/applications-setup/browsers/vivaldi.sh Outdated Show resolved Hide resolved
core/tabs/applications-setup/docker-setup.sh Outdated Show resolved Hide resolved
core/tabs/applications-setup/docker-setup.sh Outdated Show resolved Hide resolved
@lj3954 lj3954 mentioned this pull request Nov 7, 2024
1 task
@ChrisTitusTech ChrisTitusTech merged commit f400eee into ChrisTitusTech:main Nov 11, 2024
4 checks passed
jeevithakannan2 added a commit to jeevithakannan2/linutil that referenced this pull request Nov 11, 2024
* Dnf5 fixes

* again fix :)

* final fix

* added version check

* Apply suggestions from code review

Co-authored-by: Jeevitha Kannan K S <[email protected]>

* Update core/tabs/applications-setup/Developer-tools/sublime-setup.sh

Co-authored-by: Jeevitha Kannan K S <[email protected]>

---------

Co-authored-by: Jeevitha Kannan K S <[email protected]>
@Angxddeep Angxddeep deleted the Testing-#1 branch November 12, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants