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

run autoreconf #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rawlingsr
Copy link

I ran autoreconf --install after an autotools mismatch error, and a stackoverflow answer recommended it.

https://stackoverflow.com/questions/3096989/libtool-version-mismatch-error%E2%80%8B

@Vogtinator
Copy link
Owner

IMO the better option is to remove all generated files from the repo and require that autoreconf -fi is run before building.

Or switching to a better build system like meson or cmake...

@rawlingsr
Copy link
Author

I'm a bit new to C and C++ build tools, to be honest. Maybe I should first describe what I'm trying to do. As of right now, The author of n-link seems to have moved on from maintaining the project, but I would still like to have an easy to use front end directly on my PC where I can move files back and forth to my calculator and do other simple things.

https://lights0123.com/n-link/

The appimage no longer works, due to an incompatible version of openssl, and compiling it from source no longer works because of various out of date dependencies. I would like to reuse the work they've already put in here, whether or not they are available to look at whatever I come up with. I figured a good place to start would be updating the dependencies of libnspire-rs, one of which is libnspire itself. It looks like the original author copied and pasted code out of this repo, and then made whatever changes they needed to build libnspire-rs in few commands, and while that probably saved time, it doesn't make it clear what steps are needed each time changes to the original library are brought in.

I'm doing this in part as a learning exercise, so if moving the configuration step, or other steps, into a tool like meson is helpful, I can look into it. It's also possible that I'm barking up the wrong tree, and that there exists a native rust crate that already does everything ./configure would do here.

@Vogtinator
Copy link
Owner

I'm a bit new to C and C++ build tools, to be honest. Maybe I should first describe what I'm trying to do. As of right now, The author of n-link seems to have moved on from maintaining the project, but I would still like to have an easy to use front end directly on my PC where I can move files back and forth to my calculator and do other simple things.

https://lights0123.com/n-link/

The appimage no longer works, due to an incompatible version of openssl, and compiling it from source no longer works because of various out of date dependencies. I would like to reuse the work they've already put in here, whether or not they are available to look at whatever I come up with. I figured a good place to start would be updating the dependencies of libnspire-rs, one of which is libnspire itself. It looks like the original author copied and pasted code out of this repo, and then made whatever changes they needed to build libnspire-rs in few commands, and while that probably saved time, it doesn't make it clear what steps are needed each time changes to the original library are brought in.

What you can do in those cases is try to find which version of libnspire was imported and then do a diff to the bundled version to find what got changed.

It probably includes #3 + #4 at least.

I'm doing this in part as a learning exercise, so if moving the configuration step, or other steps, into a tool like meson is helpful, I can look into it. It's also possible that I'm barking up the wrong tree, and that there exists a native rust crate that already does everything ./configure would do here.

I don't really know much about how libnspire-rs and n-link are built. FWICT though the libnspire-sys crate does not even need configure, it handles the needed steps itself: https://github.com/lights0123/libnspire-rs/blob/main/libnspire-sys/build.rs

@rawlingsr
Copy link
Author

I copied and pasted the code from here into my local copy of that repo, but it had build errors, like missing config.h files. I guess those are added when one runs ./configure, and maybe they hardcoded those headers or something?

I'll take a closer look at the diff, especially changes on libnspire-rs that aren't present here.

@adriweb
Copy link

adriweb commented Feb 15, 2024

FWIW, I built the CLI version just a few weeks ago pretty much as-is, and it worked just fine (in fact, it's better than the current release which wasn't up-to-date with the code).
See here (works in CI for macOS and Linux, not windows but this I neither know nor care how to fix :P) https://github.com/adriweb/n-link/actions/runs/7332883847

So, removing the CLI part (I saw something like that somewhere in a PR?) would probably not be a good idea...

@Vogtinator
Copy link
Owner

I copied and pasted the code from here into my local copy of that repo, but it had build errors, like missing config.h files. I guess those are added when one runs ./configure, and maybe they hardcoded those headers or something?

Yes, I guess so. config.h isn't that much used, just in src/endianconv.h where it got replaced by a different approach: https://github.com/lights0123/libnspire-rs/blob/098b3f5fdc09a5b5b0d97688672c10d34365786c/libnspire-sys/libnspire/src/endianconv.h

Arguably little endian can even be assumed meanwhile, outside of PowerPC and s390 mainframes it's pretty much all LE.

I'll take a closer look at the diff, especially changes on libnspire-rs that aren't present here.

@rawlingsr
Copy link
Author

@adriweb I replied over in lights0123/n-link#28 to try not to get too far off track here.

@Vogtinator I hate to hard code something that's currently being generated in a portable way. My intention is that on my branch this repo would be a submodule, so that the configure step is part of a build script, which would be easier for others to inspect or modify in the future.

@Vogtinator
Copy link
Owner

@Vogtinator I hate to hard code something that's currently being generated in a portable way. My intention is that on my branch this repo would be a submodule, so that the configure step is part of a build script, which would be easier for others to inspect or modify in the future.

That would indeed be ideal, but you don't even need to run configure to be able to do that, just replicating what configure would do is enough. Especially if you want to preserve windows compatibility, autotools is a pain. You could write your own config.h in build.rs for instance.

@rawlingsr
Copy link
Author

@Vogtinator I think my original plan of just getting the rust crate to pull in this repo to ease any future updates might be more work than I initially estimated. The author of that other repository seems to have considered drift between the repos to be an acceptable cost for getting the stuff they wanted working working. I'm interested in the history here, because it looks like a lot of activity started happening over there around the same time as some of the most recent commits over here.

In any case, despite cmake and meson being the build tools which are easier to use from here on, is there any reason not to update the existing configuration script here, other than needing to basically make the same change every few years?

@Vogtinator
Copy link
Owner

In any case, despite cmake and meson being the build tools which are easier to use from here on, is there any reason not to update the existing configuration script here, other than needing to basically make the same change every few years?

Files generated by autoreconf should ideally not be part of the repo, only release tarballs.

Otherwise every change of the input files produces a massive diff and for PRs like this it's essentially unreviewable...

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.

3 participants