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

upgrade perforce packages: p4, p4v #110168

Merged
merged 1 commit into from May 5, 2022
Merged

upgrade perforce packages: p4, p4v #110168

merged 1 commit into from May 5, 2022

Conversation

corngood
Copy link
Contributor

@corngood corngood commented Jan 20, 2021 •
edited

Motivation for this change

This still doesn't have a permanent url for the specific minor version, and will break whenever they update. :(

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
    • 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.

@SuperSandro2000
Copy link
Member

Download link is no longer available.

@corngood
Copy link
Contributor Author

Yeah, looks like they mysteriously recalled 20.2:

https://www.perforce.com/support/software-release-index#tab-panel-6208

Release 2020.2 *Temporarily Unavailable for Download

@SuperSandro2000
Copy link
Member

What now? Update to a newer version? Update the URL? Can this PR be closed?

@corngood
Copy link
Contributor Author

corngood commented Apr 5, 2021

Note: The 20.2 release of Helix Core Suite has been temporarily removed as we perform some Root Cause Analysis on customer reported issues.

Once they post a new version, I'll update this PR.

@SuperSandro2000 SuperSandro2000 added the 2.status: wait-for-upstream Waiting for upstream fix (or their other action). label Apr 5, 2021
@SuperSandro2000 SuperSandro2000 marked this pull request as draft April 5, 2021 17:33
@corngood corngood changed the title p4: 2020.1.2007551 -> 2020.2.2057778 upgrade perforce packages: p4, p4v Sep 10, 2021
@corngood
Copy link
Contributor Author

I switched both of these to archive.org, as suggested in #131292.

@corngood corngood marked this pull request as ready for review September 10, 2021 00:41
@ofborg ofborg bot requested review from nathyong and nioncode September 10, 2021 00:47
nioncode
nioncode approved these changes Sep 10, 2021
View reviewed changes
Copy link
Member

@nioncode nioncode left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@corngood
Copy link
Contributor Author

Updated again, and applied your suggestions @SuperSandro2000.

@ofborg ofborg bot requested a review from nioncode November 18, 2021 03:07
SuperSandro2000
SuperSandro2000 reviewed Nov 24, 2021
View reviewed changes
pkgs/applications/version-management/p4v/default.nix
Comment on lines +22 to +26
#include <stddef.h>
int CRYPTO_set_mem_functions(
void *(*m)(size_t, const char *, int),
void *(*r)(void *, size_t, const char *, int),
void (*f)(void *, const char *, int)) { return 1; }
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel comfortable merging this with this patch. I don't know if it breaks crypto or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try to build a modified Qt instead. p4v is still a binary blob either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested latest with this hack disabled, and it still fails. The error you get is:

Connect to server failed; check $P4PORT.
Programming error: CRYPTO_set_mem_functions(): Could not set OpenSSL allocation functions.

@nathyong
Copy link
Contributor

Trying to test this now, but I don't seem to be able to download the sources from archive.org.

@corngood
Copy link
Contributor Author

@nathyong What's the problem exactly? I found archive.org extremely slow, and p4v is like 130m. Any other ideas for mirrors are welcome.

@aakropotkin aakropotkin mentioned this pull request Dec 2, 2021
Closed
13 tasks
@nathyong
Copy link
Contributor

I wasn't able to get any bytes from archive.org in the past. Seems fine (if a bit slow) now though.

@SuperSandro2000
Copy link
Member

If the download link is not changing over time and is not gone again we shouldn't use web.archive.org.

@corngood
Copy link
Contributor Author

The download link does change, unfortunately. Perforce only provides a link to the minor version, which always points to the latest patch release.

Also, I had a good look at using the shipped qt libs in case it allowed the crypto hack to be removed, but things got hacky very quickly and I wasn't able (so far) to fix a crash in qt web engine. It was feeling like it would be a worse outcome.

@corngood corngood mentioned this pull request Dec 28, 2021
Merged
13 tasks
@Artturin
Copy link
Member

Artturin commented May 1, 2022

@ofborg build p4v

@Artturin
Copy link
Member

Artturin commented May 5, 2022

files that should not be in bin are in bin

[nix-shell:~/.cache/nixpkgs-review/pr-110168]$ ls ./results/p4v/bin
p4admin  p4admin.bin  p4merge  p4merge.bin  p4v  p4v.bin  p4vc  qt.conf  QtWebEngineProcess

@corngood
Copy link
Contributor Author

corngood commented May 5, 2022

@Artturin yeah, that's not ideal, but it's unchanged in this upgrade and it's how they're distributed upstream.

@Artturin
Copy link
Member

Artturin commented May 5, 2022

did not test

@Artturin Artturin merged commit 1808791 into NixOS:master May 5, 2022
This was referenced Sep 6, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@nathyong nathyong Awaiting requested review from nathyong

@nioncode nioncode Awaiting requested review from nioncode

Assignees
No one assigned
Labels
2.status: wait-for-upstream Waiting for upstream fix (or their other action). 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12.approvals: 1 12.approved-by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@corngood @SuperSandro2000 @nathyong @Artturin @nioncode