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

ocl-icd: 2.2.10 -> 2.2.12 and move to GitHub #64201

Closed
wants to merge 4 commits into from
Closed

ocl-icd: 2.2.10 -> 2.2.12 and move to GitHub #64201

wants to merge 4 commits into from

Conversation

gloaming
Copy link
Contributor

@gloaming gloaming commented Jul 2, 2019 •
edited

Motivation for this change

The previous source forge.imag.fr is no longer available.
Builds only work because the tarball is still available on cache.nixos.org!

The GitHub tarball for is different(?!), so I checked it and found it's due to build artifacts only:

$ diff -r cache github
Only in cache/ocl-icd-2.2.10: aclocal.m4
Only in cache/ocl-icd-2.2.10: build-aux
Only in cache/ocl-icd-2.2.10: config.guess
Only in cache/ocl-icd-2.2.10: config.h.in
Only in cache/ocl-icd-2.2.10: config.sub
Only in cache/ocl-icd-2.2.10: configure
Only in cache/ocl-icd-2.2.10/doc: Makefile.in
Only in github/ocl-icd-2.2.10: .gitignore
Only in cache/ocl-icd-2.2.10/m4: libtool.m4
Only in cache/ocl-icd-2.2.10/m4: lt~obsolete.m4
Only in cache/ocl-icd-2.2.10/m4: ltoptions.m4
Only in cache/ocl-icd-2.2.10/m4: ltsugar.m4
Only in cache/ocl-icd-2.2.10/m4: ltversion.m4
Only in cache/ocl-icd-2.2.10: Makefile.in
Only in cache/ocl-icd-2.2.10/tests: Makefile.in
Only in cache/ocl-icd-2.2.10/tests: package.m4
Only in cache/ocl-icd-2.2.10/tests: testsuite

The build output (edit: between the two 2.2.10 tarballs) is only trivially different:

$ diff -r old new
diff -r old/lib/libOpenCL.la new/lib/libOpenCL.la
2c2
< # Generated by libtool (GNU libtool) 2.4.2 Debian-2.4.2-1.11
---
> # Generated by libtool (GNU libtool) 2.4.6
16c16
< # Linker flags that can not go in dependency_libs.
---
> # Linker flags that cannot go in dependency_libs.
41c41
< libdir='/nix/store/4708kgfsxihx7i81sszyqnwr61h51qqk-ocl-icd-2.2.10/lib'
---
> libdir='/nix/store/5nnrrnlb5rl87k4b8jlrl1dggdrwf852-ocl-icd-2.2.10/lib'
diff -r old/lib/pkgconfig/ocl-icd.pc new/lib/pkgconfig/ocl-icd.pc
1c1
< prefix=/nix/store/4708kgfsxihx7i81sszyqnwr61h51qqk-ocl-icd-2.2.10
---
> prefix=/nix/store/5nnrrnlb5rl87k4b8jlrl1dggdrwf852-ocl-icd-2.2.10
diff -r old/lib/pkgconfig/OpenCL.pc new/lib/pkgconfig/OpenCL.pc
1c1
< prefix=/nix/store/4708kgfsxihx7i81sszyqnwr61h51qqk-ocl-icd-2.2.10
---
> prefix=/nix/store/5nnrrnlb5rl87k4b8jlrl1dggdrwf852-ocl-icd-2.2.10

However, it's still different, so we might as well upgrade to the latest version (2.2.12) at the same time.
That release was a couple of years ago and there's been a couple of typo fixes in the manual since: OCL-dev/ocl-icd@v2.2.12...b5880e5
In this case I feel it's justified to take those and simply call it 2.2.12.

Things done

I tested clinfo, scallion and (my local branch of) waifu2x-converter-cpp - all looks OK.
I consulted nix-review and it told me I needed several GB of dependencies to build 1100 packages so I gave up. (How do I find out why? The .cache file lists only 47 affected packages.)

  • 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 nix-review --run "nix-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.

matthiasbeyer
matthiasbeyer requested changes Jul 3, 2019
View reviewed changes
pkgs/development/libraries/ocl-icd/default.nix
src = fetchFromGitHub {
owner = "OCL-dev";
repo = "ocl-icd";
# Only doc changes since release version
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really necessary IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I cannot believe it if the imports of this package have changed 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I mean that there are only doc changes between 2.2.12 and b5880e5, not between 2.2.10 (what we have now) and b5880e5.

Anyway, the import changes result from the change to GitHub's 2.2.10 rather than the update - check the individual commit diffs to see what I mean. The previous tarball was dirty - the build artifacts meant we didn't need to run the bootstrap before. Now we have to do it properly - I should've been explicit about that, sorry :)
(Apparently the tarballs are supposed to include the autotool output, but they don't!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you like to proceed?

@gloaming
Copy link
Contributor Author

@ambrop72 @worldofpeace @FRidh might as well merge this together with #64945

@ambrop72
Copy link
Contributor

@ambrop72 @worldofpeace @FRidh might as well merge this together with #64945

Not sure what you mean, they do separate things and mine affects other packages, I wouldn't combine it. Since there will be merge conflicts the one merged second should just be fixed up.

@gloaming
Copy link
Contributor Author

@ambrop72 Oh, I meant that the two PRs could be merged into master at the same time, to avoid extra rebuilds.

@ambrop72
Copy link
Contributor

@gloaming I see. If this PR ready? If so then I can make a combined PR.

@gloaming
Copy link
Contributor Author

@ambrop72 No need, probably best to keep them apart.

@drewrisinger
Copy link
Contributor

@gloaming can you rebase & fix merge conflicts?
I'm with @matthiasbeyer that you should just check out the versioned tag if possible. The doc fixes don't really buy you much unless they're user-facing (no idea if they actually are). If you choose to check out a specific commit, the version should probably be changed to unstable-YYYY.MM.DD

@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 28, 2020 05:44
@stale
Copy link

stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021
@r-burns
Copy link
Contributor

r-burns commented Nov 20, 2021

superseded by #139969

@r-burns r-burns closed this Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasbeyer matthiasbeyer matthiasbeyer requested changes

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gloaming @ambrop72 @drewrisinger @r-burns @matthiasbeyer