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

DO NOT MERGE make mac make nicer #20689

Closed
wants to merge 1 commit into from

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Nov 14, 2023

...and on all other platforms, too.

BINSFX has grown beyond its original intention, and is now
misleading and unmaintainable. Replace it with more natural
target names DEFAULTBIN and REMOTEBIN.

Signed-off-by: Ed Santiago [email protected]

None

@edsantiago edsantiago requested a review from baude November 14, 2023 16:35
@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 14, 2023
@edsantiago
Copy link
Member Author

The other part of this morning's conversation was something about make validate on Mac. This has been a wild-goose chase.

  • It's possible to run GOOS=darwin make validate on Linux. Also =freebsd. This produces 500+ and 64,000+ lines of output respectively. I can't tell if this is a problem with missing libraries/packages on Linux, i.e., I have no way of knowing if this is what happens on the respective platforms.
  • I surmise that it's possible to run GOOS=linux make validate on Mac but
    • I have no idea whether it will work or not, and
    • Is that really desirable in the first place?

If make validate, when run on a Mac, is as broken as GOOS=darwin make validate is on Linux, and if GOOS=linux make validate yields useful results on a Mac (someone please test this), then maybe it's a good idea to hardcode GOOS=linux into the Makefile for make validate. This is something I can't test.

Copy link
Contributor

openshift-ci bot commented Nov 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, edsantiago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@baude
Copy link
Member

baude commented Nov 15, 2023

will test now ...

@baude
Copy link
Member

baude commented Nov 15, 2023

for failures, there seems to be a precedent instead of echo in the Makefile for:

$(warning test goes here ...

should we follow suit or make the previous usage echos?

@baude
Copy link
Member

baude commented Nov 15, 2023

the update here is that make podman is not being "trapped" with this patch ... it is not obvious to me why not, but i can try to keep poking at this

@edsantiago
Copy link
Member Author

Re: $(warning -- yes, totally, for make docs.

For bin/podman, though, I'm leaning more and more toward just silently redirecting it to $(MAKE) bin/podman-remote or something that quietly works. No warnings, no echo, just make it happen. It just seems kinder in every sense.

@edsantiago
Copy link
Member Author

the update here is that make podman is not being "trapped" with this patch ...

Can you paste or link to the full output?

@edsantiago
Copy link
Member Author

OH! I think I got it. Gimme a while to confirm and look for a fix...

@edsantiago edsantiago force-pushed the macintosh_make branch 2 times, most recently from 905afc8 to 868e7c3 Compare November 15, 2023 16:54
@edsantiago
Copy link
Member Author

Try this. (Relevant change involves recognizing $(SRCBINDIR). I think this will work on Linux as well as Mac, but this is a trickier change that I need to test a lot more. ITM, please see if it helps on Mac.

@edsantiago
Copy link
Member Author

@baude here's a much longer, much trickier delta that I think may be overall more maintainable in the future. Could you test it at your convenience please? I'm a little concerned that we might see make: duplicate target warnings and if we do I have no idea how to fix them. So let's hope we don't.

@edsantiago edsantiago changed the title [CI SKIP] DO NOT MERGE make mac make nicer DO NOT MERGE make mac make nicer Nov 20, 2023
...and on all other platforms, too.

BINSFX has grown beyond its original intention, and is now
misleading and unmaintainable. Replace it with more natural
target names DEFAULTBIN and REMOTEBIN.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago
Copy link
Member Author

sys failure is a flake, #10927, one of our oldest most beloved ones. No point in rerunning it though, my goal here was to confirm that make still works with these changes.

@edsantiago
Copy link
Member Author

This never got any traction. I don't know if the original problem is still a problem, and without a Mac I don't even have any way to test this. I'll just assume the problem has been fixed.

@edsantiago edsantiago closed this May 14, 2024
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 13, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants