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

Fix bug when installing in GNU Octave v7.1.0 #95

Merged
merged 1 commit into from
Aug 26, 2022
Merged

Fix bug when installing in GNU Octave v7.1.0 #95

merged 1 commit into from
Aug 26, 2022

Conversation

gkourachanis
Copy link
Contributor

@gkourachanis gkourachanis commented Jul 9, 2022

Fixes #91

Some functions' names have had their name changed in "libinterp/corefcn/event-manager.cc" of the GNU Octave source code.

I've initially found out about the issue here (after facing the problem myself, as well) :
https://octave.discourse.group/t/problem-installing-tablicious-package-on-octave-7-1-0/2683/2

@carlosal1015
Copy link

I think that is not working over Arch Linux.
image

octave:1> pkg install https://github.com/gkourachanis/octave-tablicious/archive/master.tar.gz
octave:2> pkg load tablicious

@gkourachanis
Copy link
Contributor Author

I think that is not working over Arch Linux. image

octave:1> pkg install https://github.com/gkourachanis/octave-tablicious/archive/master.tar.gz
octave:2> pkg load tablicious

Hey, thanks for checking it. There was an 'endif' missing and I've added it with the latest commit.
Feel free to test again.

@carlosal1015
Copy link

Now is working, few warnings, no errors anymore with

Name            : octave
Version         : 7.1.0-4
Description     : A high-level language, primarily intended for numerical computations
Architecture    : x86_64
URL             : https://www.gnu.org/software/octave/
Licenses        : GPL
Groups          : None
Provides        : None
Depends On      : fftw  curl  graphicsmagick  glpk  hdf5  qhull  arpack  glu  ghostscript  sundials  gl2ps  qscintilla-qt5  libsndfile  qt5-tools  qrupdate
Optional Deps   : texinfo: for help-support in octave [installed]
                  gnuplot: alternative plotting
                  portaudio: audio support
                  java-runtime: java support
                  fltk: FLTK GUI
                  texlive-bin: for the publish command
Required By     : None
Optional For    : None
Conflicts With  : None
Replaces        : None
Installed Size  : 62.29 MiB
Packager        : Antonio Rojas <arojas@archlinux.org>
Build Date      : Fri May 20 12:40:37 2022
Install Date    : Sat Jul 9 17:06:29 2022
Install Reason  : Explicitly installed
Install Script  : No
Validated By    : Signature

image

@apjanke
Copy link
Owner

apjanke commented Jul 14, 2022

Code is looking good. I tested this under Octave 7.1.0 on macOS, and it loads up no problem, and the doc command works with the Tablicious classes I tested it on. (The help command does not work, but that is expected, because IMHO Octave's help function is kinda busted WRT third-party packages and code.)

Would you mind squashing your commits, and rewording the commit message to better describe what this change fixes? In the commits for this PR, I see three separate commits:

image

I'd rather those be a single commit, and instead of saying just "Update load_tablicious.m" etc, they described why the change was made, and what problem it was addressing. Like "Fix library initialization for Octave 7.x" or something like that.

Also, in the future, it'd be convenient if you had your changes for PRs on a branch in your fork that wasn't master. We can still do a master->master PR, but that gets messy in more-active repos, where there may be additional changes on the upstream master, or multiple PRs.

Do me a favor and clean this up git-wise and I'll merge it? Code here looks good; I'd just like to do the merge in a way that both keeps the Tablicious repo's git history tidy, and also preserves author credit for you.

@apjanke
Copy link
Owner

apjanke commented Jul 14, 2022

BTW, thanks for this contribution! This looks like the right approach, and it would have taken me some time to get there myself, and I'm really short on time at the moment, because Reasons.

@gkourachanis gkourachanis deleted the branch apjanke:master July 14, 2022 12:11
@gkourachanis gkourachanis deleted the master branch July 14, 2022 12:11
@gkourachanis gkourachanis restored the master branch July 14, 2022 12:13
@gkourachanis gkourachanis reopened this Jul 14, 2022
@gkourachanis
Copy link
Contributor Author

Code is looking good. I tested this under Octave 7.1.0 on macOS, and it loads up no problem, and the doc command works with the Tablicious classes I tested it on. (The help command does not work, but that is expected, because IMHO Octave's help function is kinda busted WRT third-party packages and code.)

Would you mind squashing your commits, and rewording the commit message to better describe what this change fixes? In the commits for this PR, I see three separate commits:

image

I'd rather those be a single commit, and instead of saying just "Update load_tablicious.m" etc, they described why the change was made, and what problem it was addressing. Like "Fix library initialization for Octave 7.x" or something like that.

Also, in the future, it'd be convenient if you had your changes for PRs on a branch in your fork that wasn't master. We can still do a master->master PR, but that gets messy in more-active repos, where there may be additional changes on the upstream master, or multiple PRs.

Do me a favor and clean this up git-wise and I'll merge it? Code here looks good; I'd just like to do the merge in a way that both keeps the Tablicious repo's git history tidy, and also preserves author credit for you.

Hey! Everything is fine now. BTW, I had initially commited those changes from GitHub's web interface and I couldn't find squashing option.
Anyway, I cloned it locally and properly made those changes.

BTW, thanks for this contribution! This looks like the right approach, and it would have taken me some time to get there myself, and I'm really short on time at the moment, because Reasons.

No problem! I'm glad I could help :)

Documentation widgets got their functions renamed in GNU Octave v7.1.0 release.
This commit updates those functions' names for octave-tablicious.

Signed-off-by: Georgios Kourachanis <geo.kourachanis@gmail.com>
@apjanke
Copy link
Owner

apjanke commented Jul 15, 2022

Looks good now! I'll re-test this and hopefully get it merged tonight.

@apjanke apjanke merged commit 052d3e3 into apjanke:master Aug 26, 2022
@apjanke
Copy link
Owner

apjanke commented Aug 26, 2022

Merged! Thanks for the contribution, and sorry for the slow response on my part. It's been... a week. For several weeks in a row.

@apjanke apjanke added the bug Something isn't working label Sep 1, 2022
@apjanke apjanke added this to the 0.3.7 milestone Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install tablicious 0.3.5 fails on Octave 7.x
3 participants