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.

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

Update README.md #414

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Update README.md #414

merged 5 commits into from
Dec 6, 2023

Conversation

bryce-carson
Copy link
Contributor

@bryce-carson bryce-carson commented Dec 5, 2023

  • Build the AppImage after 4.1 gets tagged
  • Upload it to GitHub releases on bryce-carson/SLiM
  • Link to it from the README.md

-[ ] AppImage still needs building and linking to from here
@bhaller
Copy link
Contributor

bhaller commented Dec 5, 2023

I take it from the unchecked checklist that I should not merge this yet. I'll proceed towards tagging the release, since it looks you're waiting on me for that now. :-> Thanks!

@bryce-carson
Copy link
Contributor Author

I take it from the unchecked checklist that I should not merge this yet. I'll proceed towards tagging the release, since it looks you're waiting on me for that now. :-> Thanks!

I had thought to merge it right away, but it does makes more sense to merge after the release. Once it is tagged, I'll work as quick as I can to get the AppImage uploaded and then edit the README again so that people know what's going on when they get here from reading the manual.

@bhaller
Copy link
Contributor

bhaller commented Dec 5, 2023

Hi @bryce-carson. The 4.1 release is now tagged; have at it! :-> Let me know when I should merge this PR. Thanks!

@bryce-carson
Copy link
Contributor Author

@bhaller Hi Ben,

I noticed that you haven't created a release for 4.1 yet; would you like to include the AppImage of SLiMgui on MesserLab/SLiM in the release, or should I release it on my fork (since it is a community binary) and it can be linked to from this repository as we've done in the past with other binary releases?

@bryce-carson
Copy link
Contributor Author

Screenshot from 2023-12-05 18-58-22

@bhaller I'm not able to get the Git SHA-1 for some reason. I made a fresh clone of the repository, checked out the v4.1 tag, and then made a clean build. Is there an option I need to enable in the cmake build to have the git commit included?

@bryce-carson bryce-carson mentioned this pull request Dec 6, 2023
6 tasks
@bhaller
Copy link
Contributor

bhaller commented Dec 6, 2023

Hi @bryce-carson! I have only made the tag, not declared it as a release, yes. I declare the release on GitHub at the point when the release has been officially announced on slim-discuss. Before that can happen, I need to do things like post the new manual PDFs and the new macOS installer; I'm presently working on those sorts of tasks. :->

I guess you're just talking about adding the AppImage to the list of release items under Assets, like here https://github.com/MesserLab/SLiM/releases/tag/v4.0.1 ? It's a question of where it gets hosted, really, right? Whether GitHub hosts it for us, or you host the file? I think maybe it is better for you to host the file – that way you can get the AppImage posted and tested before I announce the release (ideally)? But I see benefits to putting it on GitHub with all the other assets, too. Hmm. What do you think? If we wanted to include it in the GitHub assets, how would we stage the release exactly?

@bryce-carson
Copy link
Contributor Author

Hi @bryce-carson! I have only made the tag, not declared it as a release, yes. I declare the release on GitHub at the point when the release has been officially announced on slim-discuss. Before that can happen, I need to do things like post the new manual PDFs and the new macOS installer; I'm presently working on those sorts of tasks. :->

I guess you're just talking about adding the AppImage to the list of release items under Assets, like here https://github.com/MesserLab/SLiM/releases/tag/v4.0.1 ? It's a question of where it gets hosted, really, right? Whether GitHub hosts it for us, or you host the file? I think maybe it is better for you to host the file – that way you can get the AppImage posted and tested before I announce the release (ideally)? But I see benefits to putting it on GitHub with all the other assets, too. Hmm. What do you think? If we wanted to include it in the GitHub assets, how would we stage the release exactly?

It will be hosted on GitHub either way; the question is if you'd prefer it released as an asset on my fork of SLiM, since it's a community binary, or if you'd rather include it as an asset yourself.

For staging, there's nothing to do. I'd just upload the file here in this PR and then you could download it and then upload it as an asset when you make the release.

I'm thinking it might make a little more sense to keep it on my fork, rather than here; I'll go ahead with that. Sometimes thinking aloud helps 🦆.

@bhaller
Copy link
Contributor

bhaller commented Dec 6, 2023

@bhaller I'm not able to get the Git SHA-1 for some reason. I made a fresh clone of the repository, checked out the v4.1 tag, and then made a clean build. Is there an option I need to enable in the cmake build to have the git commit included?

Hmm, I don't think so? It should be automatic, but it might be fragile for some reason. In the CMakeLists.txt file we have:

# obtain the Git commit SHA-1; see ./cmake/_README.txt and https://stackoverflow.com/a/4318642/2752221
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/")
include(GetGitRevisionDescription)
get_git_head_revision(GIT_REFSPEC GIT_SHA1)
#message(STATUS "GIT_SHA1 is ${GIT_SHA1}")

You could uncomment that message line to see whether CMake is seeing the value. The GetGitRevisionDescription script is in the cmake directory of the SLiM repo. It then finds its way into SLiMgui via the files GitSHA1.h and GitSHA1.cpp.in, which are also in the cmake folder. The _README.txt file in the cmake folder has a brief overview.

The only requirement, I think, is that the repo you're building from be a proper git repo. If you just download the source files, rather than doing a git checkout, the git information is not there, and so it doesn't have a way to get the SHA-1 tag. Could that be the issue?

@bhaller
Copy link
Contributor

bhaller commented Dec 6, 2023

I'm thinking it might make a little more sense to keep it on my fork, rather than here; I'll go ahead with that. Sometimes thinking aloud helps 🦆.

OK, sounds good. I don't see much of a user-visible difference either way, so I'll follow your lead. We can always revisit this decision in future, for later releases. Either way, it would certainly be nice to have it listed as an asset. It's too bad the AppImage can't include slim and eidos also, so that it could be a one-stop solution. But I guess that form of packaging is designed only for apps, not for command-line tools; and as the blurb you put in the manual says, it isn't designed to contain >1 executable, either, I guess.

@bhaller
Copy link
Contributor

bhaller commented Dec 6, 2023

Hi @bryce-carson. Error:

Merge attempt failed
Head branch was modified. Review and try the merge again.

Not sure why; did you start this PR before the final commits for the release? If so, I think you need to rebase it.

@bryce-carson
Copy link
Contributor Author

Hi @bryce-carson. Error:

Merge attempt failed
Head branch was modified. Review and try the merge again.

Not sure why; did you start this PR before the final commits for the release? If so, I think you need to rebase it.

Yeah, I did!

I'll checkout this PR and sort it out ASAP.

@bhaller
Copy link
Contributor

bhaller commented Dec 6, 2023

@bryce-carson If you're not a git pro (I'm a git noob, personally :->), the easiest thing in this situation is to make a new PR based on the current state of the master branch, and then copy your one modified file in and commit/push. Rebasing always scares me.

@bryce-carson
Copy link
Contributor Author

@bhaller I rebased the commits and pushed the changes to my fork's master branch. You should be able to merge them now, I believe.

@bhaller bhaller merged commit 647048b into MesserLab:master Dec 6, 2023
16 checks passed
@bhaller
Copy link
Contributor

bhaller commented Dec 6, 2023

Yep, merged! Thanks; this PR is done, goodbye.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants