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

Reset35: Restore merging of #35 + fixes #49

Merged
merged 31 commits into from
Oct 16, 2019
Merged

Reset35: Restore merging of #35 + fixes #49

merged 31 commits into from
Oct 16, 2019

Conversation

leamas
Copy link
Contributor

@leamas leamas commented Oct 12, 2019 •
edited
Loading

This PR:

It builds successfully in 8 builders for me. . If you choose to accept this PR I'd recommend the following for the continued work:

  • Before merging: make sure CLOUDSMITH_API_KEY is available as an environment variable
    in appveyor and circleci builders.
  • Don't merge anything until it's verified that everything builds (I'm ready to submit PRs
    should it be required for this).
  • After this: don't merge anything which breaks the builds.
  • EDIT: Make a request for a MacOS builder at circleci.com to get the circleci MacOS running.
  • Don't push -f
  • Keep the history linear by requiring submitters like @jongough and me to rebase their PR
    branches to current master before you merge them
  • Don't do anything with the od-fixes branch until it is handled by @jongough.

Yes, this PR is invasive and heavy-handed, agreed. Given the situation it's the best I can do, though. Sorry.

@leamas
Revert to d474c8f, before merging #35
833b725
@leamas
flatpak: Initial packaging.
ec4760b
@leamas
cmake: Define OpenGL lib to use (mute warning).
206b9a8
@leamas
build: Add api16/opencpn.lib, handle api version.
1871371
@leamas
build: Build a tar.gz package along with ordinary installer
753d2a3 
The standard PluginInstall has a nasty bug which uses the source
path as part of installation path. Remove, and use a simple
OpenCPN.app prefix

The loader expects the apple tarball to have paths like:

  - OpenCPN.app/Contents/PlugIns: .dylib plugin file, binaries
    and helper libs.
  - OpenCPN.app/Contents/Resources: .lproj gettext translations
    directories.

The prefix is discarded and could be anything.

Let "make package" build both the traditional installer and the
new installation tarball. Remove references to other formats,
notably rpm.
@leamas
build: give plugin a more unique name.
1860599 
Some places including bintray uses a common namespace for all
plugins. Make sure the plugin filename is unique by including
both source and target name and version.
@leamas
build: Move to C++11
25a89cd
@leamas
build: Add plugin XML metadata generation.
b99f86f
@leamas
mingw: Fix opencpn import lib
08e68b0
@leamas
build: Update to handle recent cmake
7116858 
We are using deprecated and finally failing stuff.
See: OpenCPN/OpenCPN#1452
@leamas
ci: Add appveyor msvc build.
2d9fa60
@leamas
ci: Add travis builds for macos and debian
69c5847 
Updating the key is error-prone and requires the secret key. It
also requires the travis command line utility. Using that and
the key I was finally able to update .travis.yml using

travis encrypt -x -a deploy.key -r leamas/squiddio_pi  <my key>

-x: Overwrite existing key
-r: Name of repo

Note that the repo is part of the key so after forking a new,
encrypted key must be generated.
@leamas
Avoid using travis crappy bintray integration.
4b62982 
The travis bintray integration seems just unstable. Use a raw
access to the REST API in a shellscript instead.
@leamas
Adding flatpak build.
51dea8b
@leamas
ci: Add cloudsmith deployment.
97c4b30
@leamas
ci: Add circleci configuration.
55c9753 
... and move tarball location to cloudsmith
@leamas
.gitignore
8c5dffd
@leamas
appveyor: drop secret, unused key
ba4802f
@leamas
build: flatpak: Update used branch stable -> master
39489ce
@leamas
build: Restore .travis.yml to original state.
b22dd37
@leamas
build: appveyor: restore original deployement.
04d6016
@leamas
build: circleci: Reset built branch to master only.
9feb1bd
@leamas
ci: cloudsmith: Update repo url in upload an xml metadata.
3230d33
@jongough
Copy link
Contributor

jongough commented Oct 12, 2019 via email

@leamas
Copy link
Contributor Author

leamas commented Oct 12, 2019

@jongough

Hi!

I will rebase on whatever branch/repository is deemed the best.

This was my original position as well. However, as things has evolved I'd really prefer if @mauroc first merges this PR and you, after the steps described above, rebase your changes on master as it exists at that time.

@leamas
Copy link
Contributor Author

leamas commented Oct 12, 2019

@mauroc: The circleci builds seems to fail either because I got the cloudsmith repo name wrong (please review [3230d33]) or because CLOUDSMITH_API_KEY is missing in the circleci builder environment.

It's actually the same for the appveyor build, the cloudsmith deployment fails, presumably for the same reasons.

These are things I cannot fix...

@leamas
Copy link
Contributor Author

leamas commented Oct 12, 2019 •
edited
Loading

@jongough : At a closer look I see that the CLOUDSMITH_API_KEY variable is indeed there. Question then becomes if it is the right one, or something outdated?

EDIT: Of course this is to @mauroc ... sorry

@rgleason
Copy link
Contributor

rgleason commented Oct 12, 2019 •
edited
Loading

Good Morning,

Alec has been busy. Nice, the revert & commits in one package. This approach should leave us with a good history, identifying Plugin_Manager commits and Jon's Improvements Commits in an orderly fashion. Thank you.

  • Restores the github deployment part in appveyor.yml to d474c8f. <---Yes it does that.
  • Restores travis.yml to d474c8f <--- There is no travis.yml in this, Look under "Browse Files" upper left". But this can be fixed easily, and perhaps Alec's subsequent commits above create a travis.yml
  • The 4 ci builds aren't working here, but that must need Mauro's configuration information to be given to Alec. (perhaps a new cloudsmith key?)
  • Mauro is not to merge anything unless it builds properly (green checks on everything including ci!)
  • Leamas is ready to make a PR for any changes necessary to make everything build with a green check.
  • Once all builds have a green check, Mauro can merge.

@rgleason
Copy link
Contributor

rgleason commented Oct 12, 2019 •
edited
Loading

@leamas
Can't help asking, to learn.
After creating a new branch called reset35 based on mauro's current master, and reverting to "Revert to d474c8f, before merging #35"
What git commands/techniques did you use to apply the 22 commits on Oct 12?

@rgleason
Copy link
Contributor

rgleason commented Oct 12, 2019 •
edited
Loading

To answer my own question:
How to avoid rebasing by reverting, if possible.
Very good explanation of the perils towards the bottom.
https://git-scm.com/book/en/v2/Git-Branching-Rebasing
also https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase

On local github/squiddio_pi repository

Check out the "experiment" branch, revert to some 'commit' and then rebase it onto the "master" branch as follows:

  • git checkout master (checkout the local master branch)
  • git pull upstream master (to make local "master" match "upstream" ie "mauroc/squiddio_pi/master")
  • git checkout -b reset35 (make a new branch the same as master)
  • git revert d474c8f (revert to just after "Merge pull request Updated pot file #33 from jongough/od_fixes" on July 1)
  • git rebase ?? (Now I don't think this was necessary.)

Found a branch "work" and Comparing Changes, 24 commits done on Oct 11 and one on Oct 12.
an online comparison between mauro's master, and Alec's reset35 + work branches?
Alec's "work" and "reset35" appear to be identical except "reset35" has two other commits from today.

Which all begs the question, how did he do the git rebase? Found these leamas branches.

So, now I think I understand that he just unwound or "reverted" the master branch back to July 1 and then reopened his orginal PR that had been merged and made a new PR, then added more PR's.

No rebase necessary! Leaving that part for others.

Hope Mauro can get the ci builds working before merging.

@rgleason
Copy link
Contributor

Necessary commits since July 1 by git rebase on top of Leamas commits (when merged).

Here is a file with all the commit #s after July 1 + descriptions with notes adjacent, to help with rebase.
squiddio-pretty-oneline-of-master.txt

The commits are available in https://github.com/rgleason/squiddio_pi/commits/mauroc_mstr-fx_mkdir-oct_11

@leamas
Copy link
Contributor Author

leamas commented Oct 13, 2019

@mauroc: Still don't understand why the cloudsmith uploads fails on permission denied. Possible causes includes wrong CLOUDSMITH_API_KEY and/or something with the permissions. Attaching the permissions page for my working repo. Note that Replace Packages By Default? is ticked -- it isn't by default:

permissions

@leamas
Copy link
Contributor Author

leamas commented Oct 14, 2019

@mauroc: The first question is if it is your intent to accept or reject this PR. As I understand it, @rgleason has expressed his support for accepting it, and @jongough has declared that he can live with it. As the submitter, I personally would of course also prefer if you accept it.

But in the end, you are the upstream maintainer and it's your decision.

If you intend to merge this, we need to fix the Permission Denied cloudsmith error before merging if at all possible.

@rgleason
Copy link
Contributor 

Using $CLOUDSMITH_API_KEY: 39c8...
+ pyenv versions
 (removed)
Checking raw package upload parameters ... -�/�ERROR
Failed to validate upload parameters! (status: 403 - Forbidden)
Detail: You do not have permission to perform this action.
Exited with code 147

https://support.circleci.com/hc/en-us/articles/360002341673-Identifying-Exit-Codes-and-their-meanings
If you receive an exit code higher than 128, you have received a fatal error signal. Linux also has several standard signals. If you receive an error above 128, you must subtract 128 from that number to identify the signal.

Code 19
SIGSTOP 17,19,23 Stop Stop process

@leamas
Copy link
Contributor Author

leamas commented Oct 14, 2019

Please don't initiate discussions on trivial linux error code handling. The root cause here is the 403 error code, the rest is not relevant.

And, the issue right now is if @mauroc intends to accept or reject this PR. If he rejects it, there is nothing more to discuss. So, please don't add more noise to this PR.

@mauroc
Copy link
Owner

mauroc commented Oct 14, 2019 •
edited
Loading

And, the issue right now is if @mauroc intends to accept or reject this PR. If he rejects it, there is nothing more to discuss.

I'd certainly prefer to accept it and move on, but, as you suggest, i need to check it all builds correctly before I do it. Problem has been 1) lack of time and 2) lack of sufficient experience dealing with PRs. Any suggestions as to how to test the PR locally before I accept it?

EDIT: never mind - I didn't see the "command line instructions" link at the bottom....I'll give that a try - unless you have better advice

@mauroc
Copy link
Owner

mauroc commented Oct 15, 2019

@mauroc: The circleci builds seems to fail either because I got the cloudsmith repo name wrong (please review [3230d33]) or because CLOUDSMITH_API_KEY is missing in the circleci builder environment.

It's actually the same for the appveyor build, the cloudsmith deployment fails, presumably for the same reasons.

These are things I cannot fix...

the cloudsmith repo name
image

this is where I got the cloudsmith API key
image

This is where I put it in CircleCI
image

..and in appveyor (I just added this. In the past, I had used an encrypted key in the code)
image

@mauroc
Copy link
Owner

mauroc commented Oct 15, 2019 •
edited
Loading

@jongough : At a closer look I see that the CLOUDSMITH_API_KEY variable is indeed there. Question then becomes if it is the right one, or something outdated?

EDIT: Of course this is to @mauroc ... sorry

It looks like the cloudsmith , appveyor and circleci accounts are still valid and up to date. I have no idea why the deploys fail, but just for good measure I refreshed the Cloudsmith API key and updated the appveyor and circle ci CLOUDSMITH_API_KEY environment variables. I hope this helps

@mauroc
Copy link
Owner

mauroc commented Oct 15, 2019

  • EDIT: Make a request for a MacOS builder at circleci.com to get the circleci MacOS running.

@leamas : All I could find for a MacOS builder was a 2 week free trial for a $29 / month plan. Did you contact customer service and request a free plan ?

@mauroc
Copy link
Owner

mauroc commented Oct 15, 2019 •
edited
Loading

first the good news: the leamas-reset35 branch I am testing locally (available on mauroc/squiddio_pi as well) completed successfully on CircleCI and uploaded to Cloudsmith:

image

The not-so-good-news:
The appveyor build works, but the push to Cloudsmith fails with error :

+ cloudsmith push raw --republish --no-wait-for-sync mauro-calvi/squiddio-pi squiddio_pi-1.0.5-1_msvc-10.0.14393.tar.gz
Checking raw package upload parameters ... ERROR
Failed to validate upload parameters! (status: 401 - Unauthorized)
Detail: Invalid token.
Hint: Since you have an API key set, this probably means you don't have the permision to perform this action.
Command exited with code 145

I don't know if this permission is set in Appveyor or Cloudsmith...

Finally, the MacOS build in travis-ci fails (it used to work), but I am not sure if this is relevant any more. It is used to upload the mac release to Github ....

@rgleason
Copy link
Contributor

Mauro, Thank you for the very clear explanation showing what key goes where and how! Including Appveyor simplification!

Regarding CircleCI Cloudsmith integration
https://help.cloudsmith.io/docs/integrations-circleci
https://circleci.com/orbs/registry/orb/cloudsmith/cloudsmith

@leamas
Copy link
Contributor Author

leamas commented Oct 15, 2019

first the good news: the leamas-reset35 branch I am testing locally (available on mauroc/squiddio_pi as well) completed successfully on CircleCI and uploaded to Cloudsmith:

@mauroc: this is indeed great news!

If you look into the logs for the failing appveyor upload you will find a line like

Cannot deploy to cloudsmith, missing $CLOUDSMITH_API_KEY

Or

Using $CLOUDSMITH_API_KEY: 1234...

Where the part printed is the four leading characters in the key. Does this give any hint?

Since the circleci upload is ok there is nothing wrong with the repo and it's permissions.
As I understand it the only things to check in appveyor-upload should be the repo name and CLOUDSMITH_API_KEY. There are no security settings in appveyor involved, as long as the script sees the deployment key.

The links provided by @rgleason are not relevant. We are not using any integration, we are using the raw stuff by installing the cloudsmith cli tool and using that to deploy.

@mauroc
Copy link
Owner

mauroc commented Oct 15, 2019 •
edited
Loading

OK - Appveyor cloudsmith build/deploy now works. (for the benefit of others: there is a Save button at the bottom of the appveyor page where you enter the cloudsmith_api_key that I had not seen. I thought just pasting it into the field and exiting the field was enough to commit it)

The MacOS build/deploy also works now that I signed up for the free trial of MacOS on Circleci. (Not sure what will happen when the 2-week trial ends).

So, great progress all in all.

The macOS build on Travis-ci still fails due to problems with Homebrew setup:

/System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- active_support/core_ext/object/blank (LoadError)
	from /System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
	from /usr/local/Homebrew/Library/Homebrew/global.rb:12:in `<top (required)>'
	from /usr/local/Homebrew/Library/Homebrew/brew.rb:23:in `require_relative'
	from /usr/local/Homebrew/Library/Homebrew/brew.rb:23:in `<main>'
The command "bash ./ci/travis-build-osx.sh" exited with 1.

Is this something I need to worry about? It's been used to push a new release to Github Releases, which in turn is referenced in the opencpn.org web site, so I have to believe it will be necessary for a while the new Install feature takes over.

Other than that I am ready to accept the PR.

image

@leamas
ci: macos: Fix broken travis macos image.
3d4eed2 
Lately, the travis MacOS image homebrew installation has become
broken. Walk aroun dby re-installing homebrew.
@leamas
ci: macos: Update dependencies installation code.
fdbac6c
@leamas
Copy link
Contributor Author

leamas commented Oct 15, 2019

The MacOS errors seems to be related to changes (bugs) in the recent travis osx image. I have pushed two commits to cope with this. They build by me.

A bit curious, while we still have some fresh memories: What was the problem(s) besides the dreaded appveyor Save button? Valuable info next time we should go through this...

@leamas
Copy link
Contributor Author

leamas commented Oct 15, 2019

The MacOS build/deploy also works now that I signed up for the free trial of MacOS.

You need to get a open-source MacOS builder. See https://circleci.com/open-source/. The process was simple and fast for me.

@mauroc
Copy link
Owner

mauroc commented Oct 16, 2019 •
edited
Loading

The MacOS errors seems to be related to changes (bugs) in the recent travis osx image. I have pushed two commits to cope with this. They build by me.

looks like you fixed the issue with these 2 commits. MacOS now builds correctly in Travis-ci (and so did the other Travis-ci, appveyor and circleci builds....phew!).
Next, I will merge the leamas-reset35 branch into my master, then push it to github, and I understand that should essentially close the PR.

git checkout master
git merge leamas-reset35
git push origin master

@mauroc
Copy link
Owner

mauroc commented Oct 16, 2019

A bit curious, while we still have some fresh memories: What was the problem(s) besides the dreaded appveyor Save button? Valuable info next time we should go through this...

that and getting the opensource MacOS opensource build on CircleCI were really the only hiccups I found. The CircleCi builds went without a hitch on the first try

@mauroc mauroc merged commit fdbac6c into mauroc:master Oct 16, 2019
@leamas
Copy link
Contributor Author

leamas commented Oct 16, 2019

OK.

Thanks for how you have handled this somewhat complicated situation!

Now, I guess @jongough needs you attention so he can rebase his changes onto current master.

@leamas leamas deleted the reset35 branch October 17, 2019 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@leamas @jongough @rgleason @mauroc