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

pythonPackages.lxml: fix build on darwin #116669

Merged
merged 1 commit into from Mar 18, 2021

Conversation

bobrik
Copy link
Contributor

@bobrik bobrik commented Mar 17, 2021

Motivation for this change

Update in #107408 broke it for Darwin.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS (aarch64)
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@vcunat
Copy link
Member

vcunat commented Mar 17, 2021

I believe this needs to be rebased on staging-next (i.e. some previous commits removed) and retargeted against staging-next.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 17, 2021

I believe this needs to be rebased on staging-next (i.e. some previous commits removed) and retargeted against staging-next.

git rebase --onto=staging --onto=staging-next --onto=master HEAD~1

@bobrik
Copy link
Contributor Author

bobrik commented Mar 18, 2021

@SuperSandro2000 your command seems to rebase onto master:

$ git rebase --onto=origin/staging --onto=origin/staging-next --onto=origin/master HEAD~1
Current branch ivan/lxml-darwin is up to date.
$ git log -2
commit fd04a5e15401f1ae17791c5e0ac66cc423f0f70f (HEAD -> ivan/lxml-darwin)
Author: Ivan Babrou <github@ivan.computer>
Date:   Wed Mar 17 13:32:48 2021 -0700

    pythonPackages.lxml: fix build on darwin

commit ea68396a78b6143fc97c8319ba29c408a80faf9f (origin/master, origin/HEAD)
Merge: 3643876a3c1 59617f748e5
Author: Sandro <sandro.jaeckel@gmail.com>
Date:   Thu Mar 18 01:41:03 2021 +0100

    Merge pull request #101510 from omasanori/libressl-3.2.2

    libressl_3_2: init at 3.2.5, libressl_3_1: 3.1.4 -> 3.1.5

It may be easier to ask the bot to do the right thing.

@SuperSandro2000
Copy link
Member

/rebase staging

@github-actions
Copy link
Contributor

Failed to rebase

@bobrik
Copy link
Contributor Author

bobrik commented Mar 18, 2021

staging-next, not staging?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 your command seems to rebase onto master:

🤔 might be. Using the bot is not a bad idea.

@SuperSandro2000
Copy link
Member

/rebase staging-next

@github-actions github-actions bot changed the base branch from staging to staging-next March 18, 2021 00:49
@github-actions github-actions bot closed this Mar 18, 2021
@github-actions
Copy link
Contributor

Rebased, please reopen the pull request to restart CI

Copy link
Member

@thefloweringash thefloweringash left a comment

Choose a reason for hiding this comment

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

The problem is setupinfo.py setting _ldflags=['-isysroot', ''] and and some layer the empty string is omitted. It determines the location of the sysroot from the output of xcrun --show-sdk-path. This PR fixes it by adding an xcrun in the build environment that provides a valid path instead of the empty string, but it's also possible to patch the _ldflags out of setupinfo.py entirely and avoid the dependency. Either way works, but this requires less patching, so LGTM.

@vcunat vcunat merged commit c31077a into NixOS:staging-next Mar 18, 2021
@bobrik bobrik deleted the ivan/lxml-darwin branch April 9, 2021 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants