-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
brickstore: init at 0-unstable-2024-05-02 #309937
Conversation
Thanks for making a PR! You will need to rebase so you can comply with the Commit Conventions In your case please squash d49b7d30f35aeabdb6bc0617a09a7bf29d50ecbe 7fed93b6e68f92ede655b9bb8319b9fdb8ac1376 9d16a5fb3c26655257d84b53b3bf0a05894878ae into bb280d1bc87504ad13b77d434abe62af3996023c and reword bb280d1bc87504ad13b77d434abe62af3996023c to As well adding yourself as maintainer should be separate commit and in your case should be titled EDIT: Please move your package to |
I'm not exactly sure how package this but I think this will set you in the right direction. {
lib,
stdenv,
qt6,
libsForQt5,
fetchFromGitHub,
gst_all_1,
cmake,
libglvnd,
tbb,
ninja,
pkg-config,
}:
let
inherit (libsForQt5) qcoro;
in
stdenv.mkDerivation (finalAttrs: {
pname = "brickstore";
version = "0-unstable-2024-05-02";
src = fetchFromGitHub {
owner = "rgriebl";
repo = finalAttrs.pname;
rev = "v2024.5.2";
hash = "sha256-Bu9oNbZm3lx/CfYAReHyWe/kW+kaefDWeBtWLHOCORU=";
fetchSubmodules = true;
};
nativeBuildInputs = [
cmake
libglvnd
ninja
pkg-config
qcoro
qt6.qtdoc
qt6.qtdeclarative
qt6.qtimageformats
qt6.qtmultimedia
qt6.qtquick3d
qt6.qtquicktimeline
qt6.qtshadertools
qt6.qttools
qt6.qtwayland
qt6.wrapQtAppsHook
tbb
];
preConfigure = ''
# deletes line https://github.com/rgriebl/brickstore/blob/1e62a8bdf7da848a0033b8647d3711d95a3afd8d/cmake/BuildQCoro.cmake#L21
# This isn't good but not sure how to do it via substituteInPlace
sed -i '/^)$/d' cmake/BuildQCoro.cmake
substituteInPlace cmake/BuildQCoro.cmake \
--replace-fail 'FetchContent_Declare(' ' ' \
--replace-fail ' qcoro' ' ' \
--replace-fail ' GIT_REPOSITORY https://github.com/danvratil/qcoro.git' ' ' \
--replace-fail ' GIT_TAG v''${QCORO_VERSION}' ' ' \
--replace-fail 'FetchContent_GetProperties(qcoro)' ' ' \
--replace-fail 'FetchContent_Populate(qcoro)' ' ' \
--replace-fail \
'add_subdirectory(''${qcoro_SOURCE_DIR} ''${qcoro_BINARY_DIR} EXCLUDE_FROM_ALL)' \
'add_subdirectory(${qcoro.src} ${qcoro}bin/qcoro)'
'';
qtWrapperArgs = [
''
--prefix GST_PLUGIN_SYSTEM_PATH_1_0 : ${
lib.makeLibraryPath [
gst_all_1.gstreamer
gst_all_1.gst-plugins-base
gst_all_1.gst-plugins-good
gst_all_1.gst-plugins-bad
gst_all_1.gst-plugins-ugly
gst_all_1.gst-libav
]
}
''
];
meta = {
changelog = "https://github.com/rgriebl/brickstore/blob/main/CHANGELOG.md";
description = "BrickLink offline management tool";
homepage = "https://www.brickstore.dev/";
license = lib.licenses.gpl3Plus;
longDescription = ''
BrickStore is a BrickLink offline management tool.
It is multi-platform (Windows, macOS and Linux as well as iOS and Android),
multilingual (currently English, German, Spanish, Swedish and French), fast and stable.
'';
maintainers = with lib.maintainers; [ legojames ];
platforms = lib.platforms.linux;
};
}) |
Is this to replace my patch approach? I took that approach as I thought I read something that said that downloading addtional requirements should be done using nixos helper tools like fetchFromGithub |
Your older commits had a patch file, but it was just a duplicate of the I noticed you were downloading I was able to build the package on |
I've never had to rebase before, I've tried Would like to figure it out but would it be easier to close this pull, re-clone my fork and start again? |
No need, we can solve the issue here! Let me show you how. You will need to checkout the branch In your case this is how the rebase file should look like before saving it:
This will squash from 796ab14da982d44775144cd71d0a1271a3afd609 to 7fed93b6e68f92ede655b9bb8319b9fdb8ac1376 into 9d16a5fb3c26655257d84b53b3bf0a05894878ae After that, you will need to "softly reset" the commit by running |
I get |
That's because you typed |
I'm an idiot. Think I've done all that and now just need to push changes, which in turn requires a pull. Do I need to merge, rebase or fast-forward.
I'm guessing merge? |
No worries! Run |
I see you're struggling a bit, but no worries! So make sure to run |
Thanks, I think that's HEAD{742} based on this:
|
It's a bit hard for me to tell exactly, but it seems that |
f8ef014
to
1750925
Compare
I think I've done that now 🤞 , thanks for the help in getting this far |
Almost correct! Reset again but carefully rebase, as to not pick up anyone else commits |
Is that just down to the number of commits in the |
When I was originally asking you to run that command, there were 5 commits, hence the EDIT: You can read further about |
1a44082
to
4749c45
Compare
Think it's done |
Have I missed something? |
Please rebase and squash the other commits into e28daba898e728c78aebc2791e388f813b60f967 |
1f57e72
to
f4de587
Compare
Result of 1 package built:
|
LGTM, Thank you for sticking with the PR, even though I know it was a bit challenging! |
Yay, thanks you so much for the support, I learnt a lot (slowly) and feel more confident about adding packages going forward. What happens next with this PR, how does this become installable from nixpkgs for other users? |
Somebody with commit access has to merge it, so there are a couple of scenarios:
For future PR's that haven't been reviewed yet, you can ask on https://discourse.nixos.org/t/prs-ready-for-review/3032 or if you have Matrix account you can ask here too: https://app.element.io/#/room/#review-requests:nixos.org 1 I added a backport label, which will automatically create a backport PR that will "back port" the program to nixpkgs unstable. You can track the progress here: https://nixpk.gs/pr-tracker.html?pr=309937 When it gets merged into And final note to save time and effort for both yourself and reviewers, please take a careful look again at CONTRIBUTING.md so there is less back and forth, and future PR can get merged faster! Wishing you luck for any future PRs!Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for your PR. I've left some comments, let me know if this is clear enough.
'' | ||
]; | ||
|
||
meta = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing meta.mainProgram
|
||
src = fetchFromGitHub { | ||
owner = "rgriebl"; | ||
repo = finalAttrs.pname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use this.
repo = pname;
is nice for DRY but creates a binding that goes too far.
See further information about this here: nix-community/nixpkgs-lint#21
Please rebase your branch on top of the latest commit from |
Nope... this PR shouldn't contain any merge commit. |
d150899
to
b4e7509
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
In the future, when you have merge conflict on a PR, you can checkout master
then pull with a rebase (make sure to update your fork master first) and then do a git rebase
on your local branch.
git checkout master
# you can add `origin master` to be very explicit
git pull --rebase
git checkout add-bricktore-package
git rebase master
Then resolve any conflict
Removing redundant comments
b4e7509
to
7361668
Compare
Successfully created backport PR for |
Description of changes
Created a new package deviation for building the popular offline Lego store inventory tool Brickstore.
First version begins at v2024.5.2, see release notes
Things done
Built on local system and tested through last 2 package releases
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)I would like to support by reviewing pull requests but would like to see this pull request through to ensure I understand the process
Add a 👍 reaction to pull requests you find important.