Conversation
| return 1 | ||
| fi | ||
|
|
||
| if [ -z "${VER_NSIS}" ] |
There was a problem hiding this comment.
It's generally best practice to use double brackets [[ ]]
| VER_MAJ=$(echo "$VERSION" | cut -d. -f1) | ||
| VER_MIN=$(echo "$VERSION" | cut -d. -f2) | ||
| VER_REV=$(echo "$VERSION" | cut -d. -f3) | ||
| if [ "" == "$VER_REV" ]; then |
There was a problem hiding this comment.
Single Brackets (Dangerous):
if [ $name == "Bob" ]
If $name is empty, Bash sees: if [ == "Bob" ], which is a syntax error.
Double Brackets (Safe):
if [[ $name == "Bob" ]]
Even if $name is empty, Bash handles it gracefully without requiring extra quotes.
I try to stick to double brackets everywhere for consistency.
| VER_FULL="$VER_MAJ.$VER_MIN.$VER_REV.$VER_GITREV-$VER_DATE-$VER_GITHASH" | ||
| BUILDTIME=$(date "+%Y.%m.%d %H:%M") | ||
|
|
||
| if [ -z "${SEMVER}" ] || [ -z "${PROJECTNAME}" ] |
There was a problem hiding this comment.
I would use double brackets and you can just do "$SEMVER" and "$PROJECTNAME"
| else | ||
| # Variables passed in. Make use of them. | ||
|
|
||
| VER_FULL="${SEMVER}" |
|
I added some comments. BTW, do you think it would be better for PRs to fork the Oolite repo, then make PRs from the fork keeping the Oolite repo clean with only master and version branches (and maybe a develop branch)? |
|
Windows Test version fails to start with this error
Haven't tried the standard version. |
Added the double brackets although I am more happy with a syntax error than some automagical fix. Your idea of handing development elsewhere is interesting. I was close to suggesting you to use branches in the oolite repository, thus making your development more visible to others. But I am happy to see now we have prereleases available for download for all branches and pull requests. Which means I can make a change and everybody and download and test the behaviour even without having merged. And for guys like you who develop in private then come up with a pull request, the PR will also result in a prerelease so we can test the behaviour before having to merge. |
@oocube Did you test a PR from a forked repo with gitversion? All new devs do not have write access to the oolite repo so cannot create branches in it. I only received write access when it was required to install readthedocs. I was developing for months without any write access so forking and PRs was the only option. It is sensible for security reasons that people are not given immediate write access to the oolite repo when they start developing so we should expect that there will be PRs coming from forks like this old open one for example: #426 It should only be maintainers who have write access to the oolite repo. Not all devs also need to be maintainers. As for visibility, as long as there is a PR then there is visibility to the work done in a fork provided the user doesn't delete their branch or fork (but it's worse with full access to the oolite repo, where a user could delete a shared branch or worse the whole repo!). I think if we make branches for every small fix directly on the main Oolite repo it will be hard to find the major branches and also it will make cloning the repo slower and slower. |
|
I'm still getting that error when I download the Windows Test installer and install locally. When I build from source from this PR, it works fine. |
|
And the same error happens in the standard Windows installer. |
Errors like this occur whenever MSYS2 rebuild packages. Fixing it requires creating a new release of the Windows dependencies in the GitHub UI here: https://github.com/OoliteProject/oolite_windeps_build/releases. I reran the broken Windows test builds and they passed. I've rerun the Build Oolite package for this PR: https://github.com/OoliteProject/oolite/actions/runs/25809626925 - please see if that works for you. |
I did not, as I expect impact only to happen after this PR gets merged.
I agree not everyone should have direct write access. Security issues? Let's discuss them as they also exist with PR access. I opened #631 for that.
Let's discuss this in a different thread. This is the PR that allows us to distinguish artifacts for every branch. |
|
As I responded to the comments, the windows test problems got fixed and the discussion is drifting away from this PR's purpose I will now merge. |
3 participants
This second semver PR activates the semantic version number in our builds. With that #616 is complete.