search

sierrafoxtrot / srecord

SRecord github Mirror
https://srecord.sourceforge.net/
GNU General Public License v3.0
47 stars 23 forks source link

fixes from @marcows repo #1

Closed fenugrec closed 1 year ago

fenugrec commented 2 years ago

I was happy to learn that you are back and active maintaining srecord - great news.

Are you keeping track of the changes Markus has in his repo, to eventually merge them back into upstream ?

For example,

sierrafoxtrot commented 2 years ago

Thanks fenugrec. I thought I had pulled across all of Markus's branches. Looks like I have a few more to go!

The reference manual stuff might be tricky but I'll take a look. Definitely keen to pull in the new OS65A format too.

Thanks for the pointers. I'll log progress here.

marcows commented 2 years ago

I plan to have a look at my changes and rebase them - in the next days if possible.

sierrafoxtrot commented 2 years ago

That would be great Markus. Thank you.

Does that include the OS65A format? If not, I'll take a look at pulling that myself. Either is good for me.

marcows commented 2 years ago

The "fix-end-of-address-space" part is now ready, PR created. One of the four commits (marcows/SRecord@9aa2a7bfd53f4da1233ca7b01e8740296cedd216) had already been applied upstream in 3a7c13a14955a307685301f82661141d8e198962 - not the original patch, though, so there is another new bugfix included in the PR.

Will soon have a look at my reference manual fixes/improvements.

The OS65A stuff I don't know about, that's for someone else :) Maybe @chapmajs himself wants to push that forward?

marcows commented 2 years ago

I separated out the documentation fixes from "reference-manual-pdf-features" branch to get them out of the way for the real PDF improvements. Thereby found some more things to fix. See PR #11

I might not respond until after weekend.

marcows commented 2 years ago

Yes, porting the PDF features is a bit tricky, but it seems doable. After rebase I have a basic state now, only two problems currently.

  1. "sed '/^[.]XX ./d'" had been removed from soelim invocation during switch to cmake, but this sed invocation was extended for eliminating .pdf* macros from troff to avoid bad influence in HTML+man. Currently the URLs are missing (removed) there when generating HTML and man pages. Might I have to reactivate the sed invocation or had it been removed for a good reason, @sierrafoxtrot ?

  2. "ref-parts.so" is generated automatically now, so I have to find another way to get PDF outline hierarchy.

Will continue next week I guess.

sierrafoxtrot commented 2 years ago

Regarding item 1, wasn't removed for a good reason. I was looking to get 1.65 "good enough" to publish and so still had a backlog of minor issues including this one to work on later. Regarding ref-parts.so, it was originally automatically generated so I just kept that. If there is a good reason to go with hand-crafted I'd be keen to understand.

chapmajs commented 2 years ago

Happy to help, great project that I really couldn't live without! Is this now the official repo for SRecord? If so, I can rebase and send a PR for my OS65A stuff here.

I've been using the OS65A output generator extensively for my own projects, I feel that it's pretty solidly tested at this point.

fenugrec commented 2 years ago

Is this now the official repo for SRecord?

@sierrafoxtrot has been the maintainer of srecord on SF for years, and just recently became (quite) active again. Context here in his announcement https://sourceforge.net/p/srecord/mailman/message/37724629/

chapmajs commented 2 years ago

Just read the announcement, great to hear! I'll rebase against this repo and send a PR, if that's easier than integrating from @marcows repo.

sierrafoxtrot commented 2 years ago

That would be sensational @chapmajs! I was literally sitting in a cafe wondering how best to get in touch with you.

marcows commented 2 years ago

Regarding ref-parts.so, it was originally automatically generated so I just kept that.

Oh, indeed, it was already generated before. My SRecord repo was based on the v1.64 zip file which included ref-parts.so, so I thought it wasn't generated.

sierrafoxtrot commented 2 years ago

Regarding ref-parts.so, it was originally automatically generated so I just kept that.

Oh, indeed, it was already generated before. My SRecord repo was based on the v1.64 zip file which included ref-parts.so, so I thought it wasn't generated.

Ah, of course. There were several files that were generated by the old cook build system and just included in distribution tarball. This included ref-parts but also the makefile itself. I build the cmake based build from cook rather than the makefile. Apologies for the unexpected surprise on that one :-)

sierrafoxtrot commented 1 year ago

Hey @chapmajs, just following up to see if you'd had time to progress the rebase? I have a next release coming soon and would love to include OS65A. Completely understand that we have limited hours in the day and I'm more than happy to fill in any gaps remaining.

chapmajs commented 1 year ago

I tried to rebase but there's just too many changes to do it automatically. I'll try and get to it this week, I'll just port it across.

Thanks, Jonathan

------- Original Message ------- On Wednesday, March 29th, 2023 at 16:42, Scott Finneran @.***> wrote:

Hey @.***(https://github.com/chapmajs), just following up to see if you'd had time to progress the rebase? I have a next release coming soon and would love to include OS65A. Completely understand that we have limited hours in the day and I'm more than happy to fill in any gaps remaining.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

sierrafoxtrot commented 1 year ago

Thanks Jonathan!

chapmajs commented 1 year ago

Tried to get the current source building on Mac OS X but am having issues. I can handle it at work (Linux) but it's probably something to look at.

chapmajs commented 1 year ago

Looks like there's a requirement for CMake 3.22, but Slackware 15 is on CMake 3.21. I'm building a local CMake 3.26 but it's probably a good idea to push the requirement back to 3.21.x if possible.

fenugrec commented 1 year ago

If you edit the CMakeLists.txt cmake_minimum_required(VERSION 3.21) , does it build ?

chapmajs commented 1 year ago

It builds as well as with 3.26, which is to say, it doesn't :P

Am I doing something wrong? I'm running cmake CMakeLists.txt in the project root, followed by make.

fenugrec commented 1 year ago

Not quite. You should be doing something like

mkdir build
cd build
cmake-gui ..
(or cmake ..)
make

e.g. you want to compile in a subdir which exists separately from the source dir.

chapmajs commented 1 year ago

I can't make in the build/ directory as there's no Makefile in there. In any case, I'm getting errors like this:

/home/glitch/projects/srecord/srecord/arglex.cc:32:1: note: ‘memcmp’ is defined in header ‘<cstring>’; did you forget to ‘#include <cstring>’?
   31 | #include <srecord/versn_stamp.h>
  +++ |+#include <cstring>
   32 | 
/home/glitch/projects/srecord/srecord/arglex.cc: In member function ‘int srecord::arglex::token_next()’:
/home/glitch/projects/srecord/srecord/arglex.cc:451:31: error: ‘strchr’ was not declared in this scope; did you mean ‘std::strchr’?
  451 |             const char *eqp = strchr(arg.c_str(), '=');
      |                               ^~~~~~
      |                               std::strchr
In file included from /home/glitch/projects/srecord/srecord/arglex.cc:24:
/usr/include/c++/11.2.0/cstring:106:3: note: ‘std::strchr’ declared here
  106 |   strchr(char* __s, int __n)

Looks like there's some difference in the standard lib configs between my environments (this is on both Slackware 15 and Mac OS X) and stuff out of the std namespace aren't being pulled in as expected. Is development by chance mainly going on in Windows, by chance?

fenugrec commented 1 year ago

I can't make in the build/ directory as there's no Makefile in there.

What ? cmake creates makefiles wherever you want. What happens when you run cmake (or preferably cmake-gui .. as I wrote before) from the build-dir ?

e.g.

[21Gi] srecord $ mkdir build
[21Gi] srecord $ cd build
[21Gi] build $ cmake ..
-- The C compiler identification is GNU 12.2.1 
-- The CXX compiler identification is GNU 12.2.1  
-- Detecting C compiler ABI info
....
....
-- Configuring done (1.5s)
-- Generating done (0.1s)  
-- Build files have been written to: /home/q/d/srec/srecord/build

[21Gi] build $ make
[  1%] Building CXX object srecord/CMakeFiles/lib_srecord.dir/adler16.cc.o
....
sierrafoxtrot commented 1 year ago

Main dev environment is Linux (I'm running xubuntu but I've also seen it built recently on Fedora). Windows support has only recently become a standard build. Keen to get OS X and slackware working though.

For building do check out the BUILDING.pdf which can be fetched from the website. Summary typical instructions will be (from the toplevel directory): mkdir build && cd build && cmake .. && cmake --build . && ctest running make directly at this stage might mask a whole multitude of problems.

Suggest we sort out this cmake building issue first. (compiler is complaining for example about including files that are indeed included)

chapmajs commented 1 year ago

OK, I reset to HEAD and followed the instructions:

[glitch@mbp srecord] (master)$ pwd
/Users/glitch/projects/srecord
[glitch@mbp srecord] (master)$ git reset --hard HEAD
HEAD is now at 6bca708a Add git hash including to documentation(#55)
[glitch@mbp srecord] (master)$ mkdir build
[glitch@mbp srecord] (master)$ cd build
[glitch@mbp build] (master)$ cmake ..
-- GIT_SHA1 6bca708ae4
-- COPYRIGHT_YEARS 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2018, 2019, 2020, 2022, 2023
-- PS2PDF /usr/local/bin/ps2pdf
-- CYGPATH CYGPATH-NOTFOUND
-- Packaging for Macintosh
-- gcrypt location 
-- CMAKE_INSTALL_SYSTEM_RUNTIME_DESTINATION lib
-- CMAKE_INSTALL_PREFIX /usr
-- CMAKE_INSTALL_SYSTEM_RUNTIME_DESTINATION lib
-- CMAKE_INSTALL_PREFIX /usr
-- CMAKE_INSTALL_SYSTEM_RUNTIME_DESTINATION lib
-- CMAKE_INSTALL_PREFIX /usr
-- Configuring done (0.5s)
-- Generating done (0.3s)
-- Build files have been written to: /Users/glitch/projects/srecord
[glitch@mbp build] (master)$ ls -lah
total 0
drwxr-xr-x   2 glitch  staff    64B Apr  1 19:13 .
drwxr-xr-x  34 glitch  staff   1.1K Apr  1 19:13 ..

No Makefile in build/ as you can see. There's a Makefile generated in the parent directory, but that doesn't help me in the build/ subdirectory, and I figure if it's deviating from the usual development process then something's wrong somewhere.

Doing a make in the parent directory produces stuff like the following (just the current visible output in tmux):

In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/string_view:175:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/__string:393:62: error: cannot initialize a parameter of type 'wchar_t *' with an lvalue of type 'std::__1::char_traits<char>::char_type *' (aka 'char *')
                       : __n == 0 ? __s : (char_type*)memset(__s, to_int_type(__a), __n);
                                                             ^~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/wchar.h:154:27: note: passing argument to parameter here
wchar_t *wmemset(wchar_t *, wchar_t, size_t);
                          ^
In file included from /Users/glitch/projects/srecord/srecord/arglex.cc:24:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/cstring:69:9: error: no member named 'memcpy' in the global namespace; did you mean 'wmemcpy'?
using ::memcpy;
      ~~^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/wchar.h:152:10: note: 'wmemcpy' declared here
wchar_t *wmemcpy(wchar_t * __restrict, const wchar_t * __restrict, size_t);
         ^
In file included from /Users/glitch/projects/srecord/srecord/arglex.cc:24:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/cstring:70:9: error: no member named 'memmove' in the global namespace; did you mean 'wmemmove'?
using ::memmove;
      ~~^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/wchar.h:153:10: note: 'wmemmove' declared here
wchar_t *wmemmove(wchar_t *, const wchar_t *, size_t);
         ^
In file included from /Users/glitch/projects/srecord/srecord/arglex.cc:24:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/cstring:71:9: error: no member named 'strcpy' in the global namespace
using ::strcpy;
      ~~^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/cstring:72:9: error: no member named 'strncpy' in the global namespace
using ::strncpy;
      ~~^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/cstring:73:9: error: no member named 'strcat' in the global namespace
using ::strcat;
      ~~^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
sierrafoxtrot commented 1 year ago

Are the results any different if try again and from the build directory run cmake --build . instead of running make itself?

ie starting from scratch at HEAD do something like

mkdir build && cd build && cmake .. && cmake --build . && ctest

sierrafoxtrot commented 1 year ago

That Makefile being generated in the parent directory is definitely an issue.

I think we might have a hiccup along these lines: https://stackoverflow.com/questions/24876930/cmake-produces-output-in-parent-directory

chapmajs commented 1 year ago

That Makefile being generated in the parent directory is definitely an issue.

I think we might have a hiccup along these lines: https://stackoverflow.com/questions/24876930/cmake-produces-output-in-parent-directory

That seems to be the problem! Still building but it looks like the cache file in the parent directory was the issue.

Sorry if this is a n00b error, but I don't use CMake in any of my personal or $day_job projects!

sierrafoxtrot commented 1 year ago

Don't feel bad. CMake is wonderful but it broke my spirit several times :-). It still happens on occasion!

One tip that I never really appreciated was it can be helpful to call cmake --build . instead of make. Also, ctest instead of make test. On some platforms the default build system might be ninja or something else and doing this will push that complexity into the background. You can pass it args like -j for example so cmake -j 8 --build . kick off 8 parallel compile jobs and it will translate to the underlying build tool.

Thanks for sticking with it!

fenugrec commented 1 year ago

One tip that I never really appreciated was it can be helpful to call cmake --build .

Good point. Also note that git reset --hard doesn't delete untracked files - try git status to see for yourself.

Agreed, forgetting to delete CMakeCache files after certain types of changes, is a subtle trap that probably caught everyone at least once !

That's another advantage of building "out of source", such as inside a subdir : easier to purge and start over, without affecting the repo you're working from.

chapmajs commented 1 year ago

No problem -- thanks for keeping the project alive and working with me to get what I'm sure amounts to a rounding-error platform integrated :P We...actually generate this output for what amounts to billable work.

I have the initial work ported over:

https://github.com/glitchwrks/SRecord/tree/os65a_work

I would like to get a test written for it, but couldn't figure out the old build system (I think I was missing some critical part having to do with Ageis or whatever the old one was...something basically unsearchable due to some namespace collission IIRC). It looks like tests are much easier for the new system, so let me see if I can get that knocked out tonight.

sierrafoxtrot commented 1 year ago

The tests are such a valuable part of the project so that would be greatly appreciated! I'd also recommend checking out the "New Format" section of the reference manual (which should now be building for you). It has a handy list of files that need to be updated and are easily overlooked especially for new input formats to enable --guess to work and for inclusion in the libsrecord public interface.

chapmajs commented 1 year ago

Alright, tests run no problem now, and I have a working test! I have added to the suggested documentation. Should I add to AUTHORS too?

Currently this only generates output, as the dump format from the OSI 65A monitor is completely different. That's OK, right?

chapmajs commented 1 year ago

Also it looks like it builds on Slackware 15 with CMake 3.21, but the tests all fail as it can't find test_prelude.sh. I'll see if I can run that to ground and do a separate PR on it, as it'd be nice to build with the CMake included with Slackware 15. I can update the Slackbuild for it once the new release is out, either way.

chapmajs commented 1 year ago

Probably something to do with ENVIRONMENT_MODIFICATION which the tests are using, and appears to have been added in CMake 3.22:

https://cmake.org/cmake/help/latest/release/3.22.html

I'm going to say that fixing this in a way that handles path separation differences the way path_list_prepend does automatically probably greatly exceeds my experience with CMake and ctest. If this is something that someone else can fix without too much headache, great, I can test it on Slackware 15 for you. If not, I'll do something disgusting in the Slackbuild and sed the version back in the CMakeList.txt or something :P

sierrafoxtrot commented 1 year ago

Definitely agree we peel the cmake changes off into a separate PR to investigate. But sounds like great progress! and I'm keen to take a look when you are happy.

chapmajs commented 1 year ago

OK I'll go ahead and open a PR for it!

marcows commented 1 year ago

I have a next release coming soon

I know it has been a while without activity, but I really hoped to get PDF metadata and outline of the reference-manual-pdf-features branch integrated before a new release.

Do you have a schedule/deadline?

Amongst others, slight differences in destination position of the PDF outline links between Linux and Windows build was one reason holding me back.

sierrafoxtrot commented 1 year ago

Do you have a schedule/deadline?

No hard and fast deadline. The normal trigger for a new release used to be either a new format or a serious bug fix and we have three new formats including OS65A so I figured it was about time. But there's no hard and fast deadline so please don't panic or feel rushed.

I'd really like to understand what the changes are if that is at all possible. Is there a branch I could check out just to understand what kind of changes you'd like to make?

chapmajs commented 1 year ago

Added a file input feature for OSI 65A to the current PR. Friend suggested that maybe someone will have some SRecord output in OSI 65A format and need to read it back in (using SRecord, of course!) and turn it into something else.

marcows commented 1 year ago

I'd really like to understand what the changes are if that is at all possible. Is there a branch I could check out just to understand what kind of changes you'd like to make?

I created #58 so discussion can continue there.

marcows commented 1 year ago

This issue could be closed now.

Task "OS65A format" has been finished by #57.

Task "PDF features" has been finished by #58, though not all of the original adaptions have been adopted: