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.

Sign up for GitHub

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

coccinelle: 1.0.0-r23 -> 1.0.6 #23460

Closed
wants to merge 1 commit into from
Closed

coccinelle: 1.0.0-r23 -> 1.0.6 #23460

wants to merge 1 commit into from

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Mar 3, 2017 •
edited

Remove camlp4 dependency, documentation
says it is no longer needed.

Keeping pycaml however so it uses ours
instead of the bundled version.

Enable tests for sanity.

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
Copy link

@dtzWill, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vbgl, @amiddelk and @thoughtpolice to be potential reviewers.

@vbgl
Copy link
Contributor

vbgl commented Mar 4, 2017

Tests fail on darwin:

Testing if spatch works with python scripts...
Python error: dlopen(/nix/store/jjvnm28scpgrbm9pklmrk5373k9g2scf-python-2.7.13/lib/python2.7/lib-dynload/operator.so, 2): Symbol not found: __PyUnicodeUCS4_AsDefaultEncodedString
Referenced from: /nix/store/jjvnm28scpgrbm9pklmrk5373k9g2scf-python-2.7.13/lib/python2.7/lib-dynload/operator.so
Expected in: flat namespace
in /nix/store/jjvnm28scpgrbm9pklmrk5373k9g2scf-python-2.7.13/lib/python2.7/lib-dynload/operator.so

Maybe it’s OK to just disable tests on darwin.

Also, restricting OCaml to version 4.01 seems no longer needed (you can drop the corresponding line in all-packages.nix).

@dtzWill
Copy link
Member Author

dtzWill commented Mar 4, 2017

Thanks @vbgl I've updated with your suggestions. I also dropped the generation of the python wrappers as AFAICT they aren't needed any longer? As mentioned in the commit the wrapper was added (with doubt shortly thereafter) in 2012 so it seems likely this is indeed no longer needed. Even so, any thoughts on the matter are appreciated (I'm not sure how to test the python bits other than poking at 'pycocci') and please re-test on Darwin when you get a chance.

@dtzWill
Copy link
Member Author

dtzWill commented Mar 4, 2017

Eep, looks like 'spgen' needs a patchelf or something--it refers to the /lib/ld-linux interpreter. 'spgen.opt' is fine though:

$ file result/lib/coccinelle/spgen/spgen*
result/lib/coccinelle/spgen/spgen:     ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.24, BuildID[sha1]=a3bf1fc28c6611d0dce213eae47df8a02bfdf397, not stripped
result/lib/coccinelle/spgen/spgen.opt: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/68sa3m89shpfaqq1b9xp5p1360vqhwx6-glibc-2.25/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, not stripped

@dtzWill
Copy link
Member Author

dtzWill commented Mar 4, 2017

(Done for now, rebased to current master)

FRidh
FRidh reviewed Mar 5, 2017
View reviewed changes
pkgs/development/tools/misc/coccinelle/default.nix
# I don't have a non-NixOS system, so I cannot verify this, but shouldn't
# libpython know where to find its modules? (the path is for example in
# its Sys-module).
postInstall =
Copy link
Member

Choose a reason for hiding this comment

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

that's indeed silly. Python should always be able to find its own module...

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thanks for the confirmation!

@FRidh
Copy link
Member

FRidh commented Mar 5, 2017

I have no comments on the contents of the PR except that the commits needs to be squashed.

@dtzWill
coccinelle: 1.0.0-r23 -> 1.0.6
35a6396 
* Remove camlp4 dependency, no longer needed
* Keep pycaml to use ours over bundled version
* Enable tests for sanity (broken on darwin)
* no longer need to force specific ocaml version
* don't create python wrappers, they don't seem to be needed
* remove deprecated configure flag.
* fix spgen by removing bytecode version.
@dtzWill
Copy link
Member Author

dtzWill commented Mar 5, 2017

rebased + squashed.

@grahamc
Copy link
Member

grahamc commented Mar 6, 2017

Applied in 9614ebf, thank you!

@grahamc grahamc closed this Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

Assignees
No one assigned
Labels
8.has: package (update)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dtzWill @mention-bot @vbgl @FRidh @grahamc @joachifm