Skip to content

Fix/simplify some of our macro definitions #8152

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: 3.0-dev
Choose a base branch
from
Open

Fix/simplify some of our macro definitions #8152

wants to merge 7 commits into from

Conversation

ddstreetmicrosoft
Copy link
Contributor

@ddstreetmicrosoft ddstreetmicrosoft commented Feb 28, 2024 •
edited
Loading

+- Remove incorrect code making %check nonfatal (EDIT: this commit removed/deferred)
+- fix %_topdir macro
+- remove %_lib and %_lib64 macros (EDIT: this commit removed/deferred, with added comment to macros file)
+- remove %_docdir
+- remove redundant macros

EDIT: added commit to replace several hardcoded /usr/src/mariner uses in the toolkit

See specific commits for details.

@ddstreetmicrosoft ddstreetmicrosoft requested a review from a team as a code owner February 28, 2024 19:21
@ddstreetmicrosoft ddstreetmicrosoft force-pushed the ddstreet/macros branch 2 times, most recently from ce6611d to 152dc9e Compare February 28, 2024 19:25
@ddstreetmicrosoft ddstreetmicrosoft requested a review from a team as a code owner February 28, 2024 19:25
dmcilvaney
dmcilvaney requested changes Feb 28, 2024
View reviewed changes
SPECS/mariner-rpm-macros/macros Outdated
# hardcoded the /usr/src/mariner directory for building. So this
# adopts SUSE's approach, while using our /usr/src/mariner directory
# instead of the traditional /usr/src/packages.
%_topdir %{expand:%%global _topdir %{lua:if posix.access(rpm.expand("/usr/src/mariner"), "w") then print "/usr/src/mariner" else print "%{getenv:HOME}/rpmbuild" end} \
Copy link
Contributor

Choose a reason for hiding this comment

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

We will want to do a few tests on this one, make sure it works as expected. I can try some local tools if you queue a full end-to-end build (I'd turn on the raw toolchain boostrap as well, since that might have some knock on effects).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's pipeline run:
https://dev.azure.com/mariner-org/mariner/_build/results?buildId=516244&view=results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new pipeline build with latest push https://dev.azure.com/mariner-org/mariner/_build/results?buildId=516306&view=results

Copy link
Contributor

Choose a reason for hiding this comment

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

Found a few spots its hard-coded in the tools:
build_mariner_toolchain.sh, build_official_toolchain_rpms.sh, toolchain.mk (aside these should probably just be inheriting from each other if they truly need to be hard-coded...) @anphel31

Also the containerized build env. will need a polish up @neha170 .

I'm supportive of getting this change in as well, but at minimum the toolchain bits will need adjusting.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://dev.azure.com/mariner-org/mariner/_build/results?buildId=516389&view=results

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 updated all the places in the toolkit that grep showed it was hardcoded and tried to fix them (but @anphel31 and @neha170 should review it).

And also as you said above, we probably shouldn't be hardcoding rpm build locations into the toolkit, at least not in multiple different places.

jslobodzian
jslobodzian reviewed Feb 28, 2024
View reviewed changes
jslobodzian
jslobodzian reviewed Feb 28, 2024
View reviewed changes
@dmcilvaney
Copy link
Contributor

hash for macros should be 2fd9d7600f1c5c1abcf2c7eddba05eb52eca24be60d916163e1fedbac8abac97 by the looks of it.

@ddstreetmicrosoft
Copy link
Contributor Author

hash for macros should be 2fd9d7600f1c5c1abcf2c7eddba05eb52eca24be60d916163e1fedbac8abac97 by the looks of it.

ack, i always forget to update it since the files are checked into git and don't really need to be in the 'signatures' file.

updated in latest push

@ddstreetmicrosoft ddstreetmicrosoft requested a review from a team as a code owner February 29, 2024 10:58
@ddstreetmicrosoft ddstreetmicrosoft force-pushed the ddstreet/macros branch 6 times, most recently from a6f5e13 to e2f0948 Compare February 29, 2024 15:59
@ddstreetmicrosoft
Copy link
Contributor Author

https://dev.azure.com/mariner-org/mariner/_build/results?buildId=516943&view=results

@ddstreetmicrosoft
Copy link
Contributor Author

another pipeline run after the rebase https://dev.azure.com/mariner-org/mariner/_build/results?buildId=516985&view=results

@dmcilvaney
Copy link
Contributor

another pipeline run after the rebase https://dev.azure.com/mariner-org/mariner/_build/results?buildId=516985&view=results

Pipeline failed, but I'm not sure it's related to this PR... @anphel31 I've seen this a few times:

time="2024-02-29T18:20:09UTC" + /usr/lib/rpm/mariner/brp-python-bytecompile /usr/bin/python3 1 0
time="2024-02-29T18:20:09UTC" /usr/var/tmp/rpm-tmp.idVeEg: line 561: /usr/lib/rpm/mariner/brp-python-bytecompile: No such file or directory
time="2024-02-29T18:20:09UTC" error: Bad exit status from /usr/var/tmp/rpm-tmp.idVeEg (%install)

I don't see anything in filesystem.spec that would have affected this lately...

@ddstreetmicrosoft ddstreetmicrosoft force-pushed the ddstreet/macros branch 3 times, most recently from 77dccaa to 0f6e473 Compare March 2, 2024 03:51
@ddstreetmicrosoft
Copy link
Contributor Author

another pipeline run https://dev.azure.com/mariner-org/mariner/_build/results?buildId=518491&view=results

@dmcilvaney I think this is just waiting on your review

@ddstreetmicrosoft
azurelinux-rpm-macros: remove redundant macros
8c9f90f 
These macros are all already provided (and maintained) by the rpm
package and should not be defined in our distro-specific macros
file. This should not be a functional change since all the removed
macros are already identically defined elsewhere.
@ddstreetmicrosoft
azurelinux-rpm-macros: remove %_docdir
bb4ffd2 
The %_docdir macro is a builtin inside rpm and should not be
redefined; to override the default docdir, the %_defaultdocdir macro
should be used instead. Since we currently define it to the same
location as %_defaultdocdir we should just drop our definition.
@ddstreetmicrosoft
Copy link
Contributor Author

another rebase, and another pipeline run: https://dev.azure.com/mariner-org/mariner/_build/results?buildId=523385&view=results

@dmcilvaney i think this is just waiting on your review?

@dmcilvaney
Copy link
Contributor

@anphel31 Any idea on this one? https://dev.azure.com/mariner-org/mariner/_build/results?buildId=523388&view=logs&j=db98f19e-da46-5e4d-a4ba-372cf3771a92&t=8831c5ce-9476-5d69-9bf5-bfd7485723f1&l=225727

@dmcilvaney dmcilvaney self-requested a review March 8, 2024 19:01
@ddstreetmicrosoft ddstreetmicrosoft mentioned this pull request Mar 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jslobodzian jslobodzian jslobodzian left review comments

@dmcilvaney dmcilvaney Awaiting requested review from dmcilvaney

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ddstreetmicrosoft @dmcilvaney @jslobodzian