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

Remove agbcc #4994

Merged
merged 1 commit into from
Aug 11, 2024
Merged

Remove agbcc #4994

merged 1 commit into from
Aug 11, 2024

Conversation

mrgriffin
Copy link
Collaborator

@mrgriffin mrgriffin commented Jul 17, 2024

Title, in preparation for 1.10.

Fixes #4946

@mrgriffin mrgriffin added this to the 1.10 milestone Jul 17, 2024
@hedara90 hedara90 added the bugfix Bugfixes label Jul 26, 2024
@Bassoonian Bassoonian removed the bugfix Bugfixes label Jul 27, 2024
@AlexOn1ine
Copy link
Collaborator

is there anything left for the pr to be merged?

@mrgriffin
Copy link
Collaborator Author

I think it works and removes everything it should/doesn't remove everything it shouldn't, but a second set of eyes could be good?

@mrgriffin mrgriffin marked this pull request as ready for review August 8, 2024 16:11
@tertu-m
Copy link
Collaborator

tertu-m commented Aug 8, 2024

How does this handle if MODERN blocks in the existing codebase?

@hedara90
Copy link
Collaborator

hedara90 commented Aug 8, 2024

How does this handle if MODERN blocks in the existing codebase?

MODERN is still set to 1 in the Makefile so they should work the same

Copy link
Collaborator

@hedara90 hedara90 left a comment

Choose a reason for hiding this comment

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

The #if MODERN blocks in the code are also unnecessary now since it's always modern.
I tested compiling with those removed, and all tests pass at least

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@mrgriffin
Copy link
Collaborator Author

Good comments:

  • #if MODERN these could go. I think a lot of them come from pret? All the #if BUGFIXes do, and those are all essentially forced on under modern. I felt like that could be quite invasive, but you're welcome to PR to my PR if the invasiveness is worth it :)
  • build/modern and build/modern-test. I didn't think about these, but I think they probably shouldn't just change to what the agbcc directories are, because I think that would cause people who update to 1.10 to try and link modern-compiled and agbcc-compiled artifacts until they make clean to remove all the agbcc-compiled ones?
  • tidymodern yeah, that could be just something else. What would you want to call it? tidy currently does tidymodern and tidycheck. I suppose it could be inlined into tidy? I doubt there's much use for "delete the ROM build artifacts, but not the test build artifacts"?

@hedara90
Copy link
Collaborator

hedara90 commented Aug 9, 2024

I can PR the #if MODERN removal after this is in, keeps the invasiveness down.
Build directories, we can wait with that until 1.11 (or 1.10.x) then, by then all agbcc artifacts should be gone.
And yea, just tidy works I'd say, if someone really needs "delete the ROM build artifacts, but not the test build artifacts", they can scream at me.

@tertu-m
Copy link
Collaborator

tertu-m commented Aug 9, 2024

So to be clear I think the code in those blocks should stay around (most of it seems like things like alignment checks)

@tertu-m
Copy link
Collaborator

tertu-m commented Aug 9, 2024

Okay in that case if MODERN is set to 1 i have no issues with that.

@AsparagusEduardo AsparagusEduardo added General Doesn't fit under other labels type: refactor labels Aug 11, 2024
Copy link
Collaborator

@hedara90 hedara90 left a comment

Choose a reason for hiding this comment

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

Further changes to clean up the remnants of the agbcc/modern split can be addressed at a later time.

@hedara90 hedara90 merged commit 25f7f43 into rh-hideout:upcoming Aug 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Doesn't fit under other labels type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants