rtags: 2.3 -> 2.8 #21909

periklis · 2017-01-15T17:27:37Z

Motivation for this change

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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/)
Fits CONTRIBUTING.md.

mention-bot · 2017-01-15T17:27:38Z

@periklis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @globin, @anderspapitto and @Ralith to be potential reviewers.

Ralith · 2017-01-15T17:47:56Z

You need to update src.sha256 as well at minimum, or this won't build.

periklis · 2017-01-15T19:40:21Z

@Ralith Thx, i missed that.

Ralith · 2017-01-15T22:07:08Z

pkgs/development/tools/rtags/default.nix


  buildInputs = [ cmake llvmPackages.llvm openssl llvmPackages.clang emacs ]
-    ++ lib.optional stdenv.isDarwin apple_sdk.sdk;
+    ++ lib.optional stdenv.isDarwin [ apple_sdk.sdk apple_sdk.frameworks.CoreServices ];


s/optional/optionals/

Ralith · 2017-01-15T22:07:28Z

pkgs/development/tools/rtags/default.nix

@@ -16,11 +16,13 @@ stdenv.mkDerivation rec {
           MACOSX_DEPLOYMENT_TARGET="10.9"
  '';

+  NIX_CFLAGS_COMPILE = "-fexceptions";


Why was this added?

According to the build process rtags needs exception support.

I'm not sure what "according to the build process" means. There's multiple issues here: -fexceptions is enabled by default, overriding NIX_CFLAGS_COMPILE is the wrong way to specify cflags, and package configures scripts are in general responsible for choosing their compilation options themselves--it's unlikely that NixOS-specific customizations are needed outside of extending search paths.

On investigation, it looks like -fno-exceptions is coming from $(llvm-config --cxxflags) on line 11. If you add -fexceptions after that, and remove the NIX_CFLAGS_COMPILE line here, I'll approve these changes.

Thx for the input, i am occasionally contributing to packages, so i am interrested to learn. You mean i should put FLAGS in CFLAGS or CXXFLAGS and not NIX_CFLAGS_COMPILE, right?

The exact method will depend on the build system used by the package in question. In this case, it seems like we needed to work around an issue with the flags we were already passing in a very rtags-specific environment variable.

Ralith

Looks good to me!

periklis · 2017-01-16T21:11:54Z

Hmm... something is wrong with the build of rdm.cpp. I am getting a strange compile error on OSX:

[ 86%] Building CXX object src/CMakeFiles/rdm.dir/rdm.cpp.o
In file included from /private/var/folders/tv/h06nsg4s31b9lcd0_wl1rcjh0000gn/T/nix-build-rtags-2.8.drv-0/rtags/src/rdm.cpp:32:
/private/var/folders/tv/h06nsg4s31b9lcd0_wl1rcjh0000gn/T/nix-build-rtags-2.8.drv-0/rtags/src/RTags.h:978:5: warning: default label in switch which covers all enumeration values [-Wc
overed-switch-default]                                                                                                                                                              
    default:
    ^
/private/var/folders/tv/h06nsg4s31b9lcd0_wl1rcjh0000gn/T/nix-build-rtags-2.8.drv-0/rtags/src/rdm.cpp:318:11: error: use of undeclared identifier 'FileManagerWatch'; did you mean 'No
FileManagerWatch'?                                                                                                                                                                  
        { FileManagerWatch, "filemanager-watch", 'M', CommandLineParser::NoValue, "Use a file system watcher for filemanager." },
          ^~~~~~~~~~~~~~~~
          NoFileManagerWatch
/private/var/folders/tv/h06nsg4s31b9lcd0_wl1rcjh0000gn/T/nix-build-rtags-2.8.drv-0/rtags/src/rdm.cpp:184:5: note: 'NoFileManagerWatch' declared here
    NoFileManagerWatch,
    ^
/private/var/folders/tv/h06nsg4s31b9lcd0_wl1rcjh0000gn/T/nix-build-rtags-2.8.drv-0/rtags/src/rdm.cpp:585:14: error: use of undeclared identifier 'FileManagerWatch'; did you mean 'No
FileManagerWatch'?                                                                                                                                                                  
        case FileManagerWatch: {
             ^~~~~~~~~~~~~~~~
             NoFileManagerWatch
/private/var/folders/tv/h06nsg4s31b9lcd0_wl1rcjh0000gn/T/nix-build-rtags-2.8.drv-0/rtags/src/rdm.cpp:184:5: note: 'NoFileManagerWatch' declared here
    NoFileManagerWatch,
    ^
1 warning and 2 errors generated.

Afaik the preprocessor conditionals on https://github.com/Andersbakken/rtags/blob/master/src/rdm.cpp#L39 and https://github.com/Andersbakken/rtags/blob/master/src/rdm.cpp#L181 are quite strange combination to me. Any idea?

Ralith · 2017-01-16T21:16:15Z

Looks like an upstream (i.e. rtags) bug at a glance; perhaps they're not testing releases on OSX. I don't have time to dig deeply into it right now, but you might try contacting the maintainer.

periklis · 2017-01-17T21:55:25Z

I've opened an issue on Andersbakken/rtags #891

Mic92 · 2017-01-18T07:34:22Z

@periklis is this package good to merge or do you want to wait for upstream response regarding a fix for your problem?

periklis · 2017-01-18T09:04:57Z

@Mic92 If we merge, we will make our OSX users unhappy. If you don't bother i would like to wait for a response from upstream. Otherwise i would open an osx issue if that the way to go, what's your opinion?

Mic92 · 2017-01-18T10:23:25Z

@periklis yes. If it breaks OS X, we should wait.

periklis · 2017-01-22T10:23:10Z

@Mic92 The rtags maintainer fixed the bug in upstream and i pinned the release to that commit. I've asked for a release tag and will deliver that asap. Now we can merge this PR.

fpletz added 8.has: package (update) 9.needs: clean-up labels Jan 15, 2017

periklis force-pushed the topic_rtags branch from 9ee9980 to 113c44b Compare January 15, 2017 19:39

Ralith suggested changes Jan 15, 2017

View reviewed changes

periklis force-pushed the topic_rtags branch 2 times, most recently from 4c71793 to c762a54 Compare January 16, 2017 20:51

Ralith approved these changes Jan 16, 2017

View reviewed changes

periklis force-pushed the topic_rtags branch from c762a54 to e6386a9 Compare January 17, 2017 21:54

Mic92 removed the 9.needs: clean-up label Jan 18, 2017

periklis force-pushed the topic_rtags branch 2 times, most recently from 4ae2781 to 568b000 Compare January 21, 2017 19:32

rtags: 2.3 -> 2.8-p1

a484edc

periklis force-pushed the topic_rtags branch from 568b000 to a484edc Compare January 22, 2017 10:21

globin merged commit 01fef92 into NixOS:master Jan 22, 2017

periklis deleted the topic_rtags branch July 25, 2018 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rtags: 2.3 -> 2.8 #21909

rtags: 2.3 -> 2.8 #21909

periklis commented Jan 15, 2017 •

edited by grahamc

mention-bot commented Jan 15, 2017

Ralith commented Jan 15, 2017 •

edited

periklis commented Jan 15, 2017

Ralith Jan 15, 2017

periklis Jan 16, 2017

Ralith Jan 15, 2017

periklis Jan 16, 2017

Ralith Jan 16, 2017

Ralith Jan 16, 2017 •

edited

periklis Jan 16, 2017

Ralith Jan 16, 2017

Ralith left a comment

periklis commented Jan 16, 2017

Ralith commented Jan 16, 2017

periklis commented Jan 17, 2017

Mic92 commented Jan 18, 2017

periklis commented Jan 18, 2017

Mic92 commented Jan 18, 2017

periklis commented Jan 22, 2017

rtags: 2.3 -> 2.8 #21909

rtags: 2.3 -> 2.8 #21909

Conversation

periklis commented Jan 15, 2017 • edited by grahamc

Motivation for this change

Things done

mention-bot commented Jan 15, 2017

Ralith commented Jan 15, 2017 • edited

periklis commented Jan 15, 2017

Ralith Jan 15, 2017

Choose a reason for hiding this comment

periklis Jan 16, 2017

Choose a reason for hiding this comment

Ralith Jan 15, 2017

Choose a reason for hiding this comment

periklis Jan 16, 2017

Choose a reason for hiding this comment

Ralith Jan 16, 2017

Choose a reason for hiding this comment

Ralith Jan 16, 2017 • edited

Choose a reason for hiding this comment

periklis Jan 16, 2017

Choose a reason for hiding this comment

Ralith Jan 16, 2017

Choose a reason for hiding this comment

Ralith left a comment

Choose a reason for hiding this comment

periklis commented Jan 16, 2017

Ralith commented Jan 16, 2017

periklis commented Jan 17, 2017

Mic92 commented Jan 18, 2017

periklis commented Jan 18, 2017

Mic92 commented Jan 18, 2017

periklis commented Jan 22, 2017

periklis commented Jan 15, 2017 •

edited by grahamc

Ralith commented Jan 15, 2017 •

edited

Ralith Jan 16, 2017 •

edited