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

ngspice: update to 35 and fix issue with binary not built/installed #32403

Merged
merged 2 commits into from Aug 30, 2021

Conversation

mtboehlke
Copy link
Contributor

@mtboehlke mtboehlke commented Aug 7, 2021

General

Have the results of the proposed changes been tested?

  • I use the packages affected by the proposed changes on a regular basis and confirm this PR works for me
  • I generally don't use the affected packages but briefly tested this PR

close #32251

(edited PR title to be more clear)

Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if unnecessary, I wonder if we should make ngspice depend on libngspice... This way people's systems won't be broken.

Also, this PR needs to revbump kicad to pick up the new dependency.

Another possible solution is creating a ngspice-progs subpackage, but that feels kind of ugly.

srcpkgs/ngspice/template Outdated Show resolved Hide resolved
srcpkgs/ngspice/template Outdated Show resolved Hide resolved
srcpkgs/ngspice/template Outdated Show resolved Hide resolved
srcpkgs/ngspice/template Outdated Show resolved Hide resolved
@mtboehlke
Copy link
Contributor Author

I included the revbump to kicad.

What exactly would happen if we don't have ngspice depend on libngspice?

@mtboehlke mtboehlke marked this pull request as ready for review August 10, 2021 16:47
@mtboehlke mtboehlke requested a review from ericonr August 10, 2021 16:49
@ericonr ericonr self-assigned this Aug 10, 2021
@ericonr
Copy link
Member

ericonr commented Aug 10, 2021

What exactly would happen if we don't have ngspice depend on libngspice?

Basically that someone who installed ngspice locally to use the libraries in their own projects would no longer be able to run them. That doesn't matter for most cases, since those people will usually have ngspice-devel installed as well.

@mtboehlke
Copy link
Contributor Author

I see what you're saying.

As far as the checks go, apparently since v34 you have to add a .spiceinit file to the home directory containing set ngbehavior=mc. They pass on my machine after doing that.

@ericonr
Copy link
Member

ericonr commented Aug 11, 2021

You can create a pre_check exporting a custom HOME and creating the file, if you want to run the tests :)

@mtboehlke mtboehlke changed the title ngspice: update to 34 ngspice: update to 35 Aug 11, 2021
@mtboehlke
Copy link
Contributor Author

mtboehlke commented Aug 11, 2021

Well, while playing with this I noticed that version 35 has been released. Removing the erroneously included files is no longer needed. Thanks for the tip for including pre_check! The only thing I haven't included is a libngspice dependency in ngspice, since it doesn't really seem necessary and doubles the install size for those just looking to use ngspice (admittedly install size probably isn't a big deal either), but I can add it too if we want to go that way.

Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the shape this is taking, nice job :)

srcpkgs/ngspice/template Outdated Show resolved Hide resolved
srcpkgs/ngspice/template Show resolved Hide resolved
srcpkgs/ngspice/template Outdated Show resolved Hide resolved
@mtboehlke
Copy link
Contributor Author

:)

Comment on lines 18 to 33
# Configure can handle building ngspice as a library or as a binary, but not both at once.
# --with-ngshared builds the library, but not binary, and readline should not be configured for the library,
# see: https://github.com/imr/ngspice/commit/b86c85f85bbba6e45dc030af3e853edf8b9cfa3d
pre_configure() {
install -d .build_lib
cd .build_lib
../configure --with-ngshared ${configure_args/--with-readline=yes/}
}

# library built during first do_build, now build the binary
post_build() {
make_build_args= do_build
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to remove --with-readline=yes from configure_args then

Suggested change
# Configure can handle building ngspice as a library or as a binary, but not both at once.
# --with-ngshared builds the library, but not binary, and readline should not be configured for the library,
# see: https://github.com/imr/ngspice/commit/b86c85f85bbba6e45dc030af3e853edf8b9cfa3d
pre_configure() {
install -d .build_lib
cd .build_lib
../configure --with-ngshared ${configure_args/--with-readline=yes/}
}
# library built during first do_build, now build the binary
post_build() {
make_build_args= do_build
}
do_configure() {
mkdir build-lib
cd build-lib
../configure --with-ngshared ${configure_args}
mkdir build
../configure --with-readline=yes ${configure_args}
}
do_build() {
make -C build-lib ${makejobs}
make -C build ${makejobs}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied suggestion, with just minor tweaks

Comment on lines 41 to 49
post_install() {
vlicense COPYING
cd .build_lib
do_install
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And:

Suggested change
post_install() {
vlicense COPYING
cd .build_lib
do_install
}
do_install() {
make -C build-lib install
make -C build install
vlicense COPYING
}

Copy link
Contributor Author

@mtboehlke mtboehlke Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem clearer this way. I kind of liked not touching do_install, etc., to keep defaults and things, but maybe it just makes it a bit confusing at this point.

@mtboehlke
Copy link
Contributor Author

mtboehlke commented Aug 16, 2021

I am pretty happy with this version of the template. Functionally these last set of changes seem to do the same as the last, but make it clearer what it's doing. Thanks for all the helpful feedback so far!

@mtboehlke
Copy link
Contributor Author

I went ahead and did a rebase since my branch was old.

@mtboehlke mtboehlke changed the title ngspice: update to 35 ngspice: update to 35 and fix issue with binary not build/installed Aug 24, 2021
@mtboehlke mtboehlke changed the title ngspice: update to 35 and fix issue with binary not build/installed ngspice: update to 35 and fix issue with binary not built/installed Aug 24, 2021
@mtboehlke
Copy link
Contributor Author

mtboehlke commented Aug 25, 2021

@ericonr I am not a KiCad user ( or even really an ngspice user), but I was curious about the /usr/lib/ngspice/*cm files. It appears that they are code models used by the xspice extension. They can be used from KiCad, and should be made available, which this PR would break right now. My question is how best to handle that. Should they be moved to libngspice and have ngspice then depend on it as well, make another subpackage called something like ngspice-common, or maybe not split off libngspice at all? (PR reflects first option)

@mtboehlke mtboehlke changed the title ngspice: update to 35 and fix issue with binary not built/installed [WIP] ngspice: update to 35 and fix issue with binary not built/installed Aug 25, 2021
@mtboehlke mtboehlke changed the title [WIP] ngspice: update to 35 and fix issue with binary not built/installed ngspice: update to 35 and fix issue with binary not built/installed Aug 26, 2021
@ericonr
Copy link
Member

ericonr commented Aug 29, 2021

My question is how best to handle that. Should they be moved to libngspice and have ngspice then depend on it as well, make another subpackage called something like ngspice-common, or maybe not split off libngspice at all?

I'm honestly ok with the last option.

@mtboehlke
Copy link
Contributor Author

Sounds good! I went ahead and pushed changes to no longer split a libngspice package. Builds okay on my system.

@ericonr
Copy link
Member

ericonr commented Aug 30, 2021

Ok, I think this still requires a kicad rebuild for some godforsaken reason :/

I can test and confirm tomorrow.

@mtboehlke
Copy link
Contributor Author

Oh, there might be a issue if the version in the repo was linked against readline, which I'm pretty sure it is. We may want to rebuild kicad anyway just to be safe.

@ericonr
Copy link
Member

ericonr commented Aug 30, 2021

It's worse ;) it tries to dlopen libngspice.so.0.0.0, but the new file is libngspice.so.0.0.1

mtboehlke and others added 2 commits August 30, 2021 10:28
Also fixes issue with ngspice binary not being packaged and include
warning for kicad.
It hardcodes the underlying file for libngspice.so.0, which it dlopens
despite being linked against libngspice.so.0.
@ericonr ericonr merged commit c36159c into void-linux:master Aug 30, 2021
@ericonr
Copy link
Member

ericonr commented Aug 30, 2021

Thanks! I pushed the kicad change and merged.

@mtboehlke
Copy link
Contributor Author

:) Thanks ericonr!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngspice broken package
3 participants