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

icu-devel: fix build on Tiger #18696

Merged
merged 1 commit into from May 21, 2023
Merged

Conversation

evanmiller
Copy link
Contributor

Description

The icu 73.1 update does not build on Tiger due to the --enable-rpath flag that is passed. But with --disable-rpath, the build is broken due to an incorrect install_name.

No rev-bump needed since the compile fails.

See: https://trac.macports.org/ticket/67433
See: https://trac.macports.org/ticket/67428

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS x.y
Xcode x.y / Command Line Tools x.y.z

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@catap for port icu-devel.

@macportsbot macportsbot added type: bugfix maintainer: open Affects an openmaintainer port labels May 16, 2023
@catap
Copy link
Contributor

catap commented May 16, 2023

@evanmiller I really don't understand this patch because --enable-rpath and --disable-rpath only control the condition you broke.

Differences between --enable-rpath and --disable-rpath with your patch shouldn't be noticeable at all.

@catap
Copy link
Contributor

catap commented May 16, 2023

See: #18392 (comment)

@evanmiller
Copy link
Contributor Author

@catap --enable-rpath causes -Wl,-rpath to be passed. This causes a failed build on Tiger.

https://github.com/unicode-org/icu/blob/ba1c7006b7b5acd734538b9997102d7f46203576/icu4c/source/config/Makefile.inc.in#L62

@evanmiller
Copy link
Contributor Author

@catap
Copy link
Contributor

catap commented May 16, 2023

I see.

maybe backport it to upstream isn’t so bad idea, is it?

@catap
Copy link
Contributor

catap commented May 17, 2023

@evanmiller would you like to backport it to upstream? :)

@evanmiller
Copy link
Contributor Author

Per @kencu's suggestion I have updated the patch to 1) fix rpath installs 2) fix no-rpath installs and 3) disable rpath for MacPorts. This gives us a patch that we can send upstream.

@catap
Copy link
Contributor

catap commented May 20, 2023

I not sure about (3); I think that using rpath isn't bad if it works :)

I'll take closer look to this PR in the next few days.

@kencu
Copy link
Contributor

kencu commented May 20, 2023

macports has a policy/strong preference to not use rpaths unless there is no option, as it is too hard to debug issues. Almost all users and most devs have no clue how to be certain what libraries are really being used when rpaths are involved.

It’s best to configure all systems the same if possible

fixing the rpaths option in icu is a good thing, thanks Evan

@catap
Copy link
Contributor

catap commented May 20, 2023

After some additional thinking I'd like to agree that @rpath here makes things more complicated. Let keep it disabled.

@kencu may I ask you to merge this PR? It seems as ready as it can be :)

@catap
Copy link
Contributor

catap commented May 20, 2023

@evanmiller may you also bump a revision?

@mascguy
Copy link
Member

mascguy commented May 20, 2023

@evanmiller may you also bump a revision?

Okay, we'll wait to merge then.

@evanmiller
Copy link
Contributor Author

@evanmiller may you also bump a revision?

done

Copy link
Member

@mascguy mascguy left a comment

Choose a reason for hiding this comment

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

LGTM

@mascguy mascguy merged commit 1306d1e into macports:master May 21, 2023
0 of 3 checks passed
@evanmiller evanmiller mentioned this pull request May 21, 2023
12 tasks
@kencu
Copy link
Contributor

kencu commented May 22, 2023

well done all, and thanks for the upstream PR too.

@mascguy
Copy link
Member

mascguy commented May 22, 2023

well done all, and thanks for the upstream PR too.

Yep, agreed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: bugfix
5 participants