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

Jump to bottom

kdevelop: 5.2.4 -> 5.3.1, bump llvm version used from 3.8 to 7 #55015

Merged
merged 1 commit into from Feb 2, 2019
Merged

kdevelop: 5.2.4 -> 5.3.1, bump llvm version used from 3.8 to 7 #55015

merged 1 commit into from Feb 2, 2019

Conversation

aanderse
Copy link
Member

@aanderse aanderse commented Jan 31, 2019 •
edited

Motivation for this change

There have been some changes in kdevelop which prevented a simple version bump from compiling on NixOS. I've bumped the version and provided a fix so it will compile. I tested a brand new project with kdevelop and everything is working fine. Testing an existing project I had with kdevelop is not properly working.

I would appreciate anyone else who uses kdevelop to test this if possible and provide feedback. I'll try to follow up with kdevelop devs on troubleshooting to see what the issue might be and if it is specific to my project, or a problem with kdevelop 5.3.1 on NixOS (ie. this PR).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/kdevelop-in-nixpkgs/2013/1

@aanderse
Copy link
Member Author

aanderse commented Feb 2, 2019

After some digging and discussion I found the issue and it wasn't related to NixOS or my build at all. This PR is ready to merge.

@aanderse aanderse changed the title WIP: kdevelop: 5.2.4 -> 5.3.1, bump llvm version used from 3.8 to 7 kdevelop: 5.2.4 -> 5.3.1, bump llvm version used from 3.8 to 7 Feb 2, 2019
@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-january/2002/4

timokau
timokau reviewed Feb 2, 2019
View reviewed changes
pkgs/top-level/all-packages.nix
@@ -17857,7 +17857,7 @@ in
kdevelop-pg-qt = libsForQt5.callPackage ../applications/editors/kdevelop5/kdevelop-pg-qt.nix {};

kdevelop = libsForQt5.callPackage ../applications/editors/kdevelop5/kdevelop.nix {
llvmPackages = llvmPackages_38;
llvmPackages = llvmPackages_7;
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason not to use llvmPackages_latest? Is this likely to break with updates?

Copy link
Member Author

Choose a reason for hiding this comment

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

The llvm package provides the c++ parsing and analysis for kdevelop and different versions of llvm can dramatically impact how the application behaves. The decision to move from one version of llvm to another might not break compilation, but it could break functionality, so a person should specifically test any llvm bumps to avoid breaking behavior in the application.

Copy link
Member

Choose a reason for hiding this comment

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

Alright. Do you think this PR needs more testing? I don't use kdevelop personally.

@aanderse
Copy link
Member Author

aanderse commented Feb 2, 2019

At this point I've done enough testing I feel the PR is fine for merge.

Thanks!

@timokau
Copy link
Member

timokau commented Feb 2, 2019

Great, I'll take your word for it then :) Thank you!

@timokau timokau merged commit 0cb74f6 into NixOS:master Feb 2, 2019
@aanderse aanderse deleted the kdevelop branch February 2, 2019 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau timokau left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aanderse @nixos-discourse @timokau @GrahamcOfBorg