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

Make avrintel.h an internal header file for libavrdude #1686

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

stefanrueger
Copy link
Collaborator

See Issue #1683

@stefanrueger
Copy link
Collaborator Author

It is unclear to me why the CI fails here. I suspect the last CI change has introduced some problems?

@mcuee mcuee added the bug Something isn't working label Feb 14, 2024
@mcuee
Copy link
Collaborator

mcuee commented Feb 14, 2024

It is unclear to me why the CI fails here. I suspect the last CI change has introduced some problems?

Looks like a temporary problems from MSYS2. We can ignore it for now.

Installing additional packages through pacman...
  C:\Windows\system32\cmd.exe /D /S /C D:\a\_temp\setup-msys2\msys2.cmd -c "'pacman' '--noconfirm' '-S' '--needed' '--overwrite' '*' 'base-devel' 'mingw-w64-x86_64-gcc' 'mingw-w64-x86_64-cmake' 'mingw-w64-x86_64-libelf' 'mingw-w64-x86_64-libusb' 'mingw-w64-x86_64-libusb-compat-git' 'mingw-w64-x86_64-hidapi' 'mingw-w64-x86_64-libftdi' 'mingw-w64-x86_64-libserialport' 'mingw-w64-x86_64-readline' 'mingw-w64-x86_64-ncurses' 'mingw-w64-x86_64-termcap'"
  resolving dependencies...
  looking for conflicting packages...
  Packages (66) binutils-2.42-1  bison-3.8.2-5  diffstat-1.65-1  diffutils-3.10-1  dos2unix-7.5.2-1  flex-2.6.4-3  m4-1.4.19-2  make-4.4.1-1  mingw-w64-x86_64-binutils-2.42-1  mingw-w64-x86_64-brotli-1.1.0-1  mingw-w64-x86_64-bzip2-1.0.8-3  mingw-w64-x86_64-c-ares-1.26.0-1  mingw-w64-x86_64-ca-certificates-20230311-1  mingw-w64-x86_64-confuse-3.3-3  mingw-w64-x86_64-crt-git-11.0.0.r631.ga4c0c1d00-1  mingw-w64-x86_64-curl-8.6.0-1  mingw-w64-x86_64-expat-2.6.0-1  mingw-w64-x86_64-gcc-libs-13.2.0-4  mingw-w64-x86_64-gettext-runtime-0.22.4-6  mingw-w64-x86_64-gmp-6.3.0-2  mingw-w64-x86_64-headers-git-11.0.0.r631.ga4c0c1d00-1  mingw-w64-x86_64-isl-0.26-1  mingw-w64-x86_64-jsoncpp-1.9.5-3  mingw-w64-x86_64-libarchive-3.7.2-1  mingw-w64-x86_64-libb2-0.98.1-2  mingw-w64-x86_64-libffi-3.4.4-1  mingw-w64-x86_64-libiconv-1.17-4  mingw-w64-x86_64-libidn2-2.3.7-2  mingw-w64-x86_64-libpsl-0.21.5-2  mingw-w64-x86_64-libssh2-1.11.0-2  mingw-w64-x86_64-libsystre-1.0.1-5  mingw-w64-x86_64-libtasn1-4.19.0-1  mingw-w64-x86_64-libt
  Total Download Size:    19.73 MiB
  Total Installed Size:  640.22 MiB
  :: Proceed with installation? [Y/n] 
  :: Retrieving packages...
  Error: The operation was canceled.

It was ok last week.
https://github.com/avrdudes/avrdude/actions/runs/7814004042/job/21314328103

@mcuee
Copy link
Collaborator

mcuee commented Feb 14, 2024

@stefanrueger

I think I know the reason now.

Installing additional packages through pacman...
  C:\Windows\system32\cmd.exe /D /S /C D:\a\_temp\setup-msys2\msys2.cmd -c "'pacman' '--noconfirm' '-S' '--needed' '--overwrite' '*' 'base-devel' 'mingw-w64-clang-i686-gcc' 'mingw-w64-clang-i686-cmake' 'mingw-w64-clang-i686-libelf' 'mingw-w64-clang-i686-libusb' 'mingw-w64-clang-i686-libusb-compat-git' 'mingw-w64-clang-i686-hidapi' 'mingw-w64-clang-i686-libftdi' 'mingw-w64-clang-i686-libserialport' 'mingw-w64-clang-i686-readline' 'mingw-w64-clang-i686-ncurses' 'mingw-w64-clang-i686-termcap'"
  error: target not found: mingw-w64-clang-i686-libusb
  error: target not found: mingw-w64-clang-i686-libusb-compat-git
  error: target not found: mingw-w64-clang-i686-libftdi
  Error: The process 'C:\Windows\system32\cmd.exe' failed with exit code 1

MSYS2 started to drop 32bit packages. I will create a new PR to drop MSYS2 mingw32 and clang32 build.

  1. News:
    https://www.msys2.org/news/
    2023-12-13 - Starting to drop some 32-bit Packages

  2. Example Dropped package: you can see that no more 32bit packages for libusb.
    https://packages.msys2.org/base/mingw-w64-libusb

Binary Packages:
mingw-w64-clang-x86_64-libusb
mingw-w64-clang-aarch64-libusb
mingw-w64-x86_64-libusb
mingw-w64-ucrt-x86_64-libusb

  1. Same for the following packages.
    https://packages.msys2.org/base/mingw-w64-libusb-compat-git
    https://packages.msys2.org/base/mingw-w64-libftdi

@mcuee
Copy link
Collaborator

mcuee commented Feb 14, 2024

@stefanrueger

CMake change seems to be missing so that additional internal include file ( libavrdude-avrintel.h) is not installed. But I do not know if this is the right fix or not.

PUBLIC_HEADER "libavrdude.h;libavrdude-avrintel.h"

Example installation under MSYS2 mingw64:

MINGW64 /c/work/avr/avrdude_test/avrdude_sr
# cmake --build build_mingw64_nt-10.0-19045 --target install
[0/1] Install the project...-- Install configuration: "RelWithDebInfo"
-- Installing: C:/Program Files (x86)/avrdude/bin/avrdude.exe
-- Installing: C:/Program Files (x86)/avrdude/lib/libavrdude.a
-- Installing: C:/Program Files (x86)/avrdude/include/libavrdude.h
-- Installing: C:/Program Files (x86)/avrdude/etc/avrdude.conf
-- Installing: C:/Program Files (x86)/avrdude/share/man/man1/avrdude.1

@mcuee
Copy link
Collaborator

mcuee commented Feb 14, 2024

@Youw

Just wondering if you think the original suggested fix is still valid or not now that libavrdude-avrintel.h is not exactly public header file but still needed. Thanks.

MINGW64 /c/work/avr/avrdude_test/avrdude_sr
$ git diff
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index c02d4d54..9c1b9149 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -278,7 +278,7 @@ add_library(libavrdude

 set_target_properties(libavrdude PROPERTIES
     PREFIX ""
-    PUBLIC_HEADER "libavrdude.h"
+    PUBLIC_HEADER "libavrdude.h;libavrdude-avrintel.h"
     VERSION 1.0.0
     SOVERSION 1
     )

(Note: run as admin under Windows)
# cmake --build build_mingw64_nt-10.0-19045 --target install
[0/1] Install the project...-- Install configuration: "RelWithDebInfo"
-- Installing: C:/Program Files (x86)/avrdude/bin/avrdude.exe
-- Installing: C:/Program Files (x86)/avrdude/lib/libavrdude.a
-- Up-to-date: C:/Program Files (x86)/avrdude/include/libavrdude.h
-- Installing: C:/Program Files (x86)/avrdude/include/libavrdude-avrintel.h
-- Installing: C:/Program Files (x86)/avrdude/etc/avrdude.conf
-- Up-to-date: C:/Program Files (x86)/avrdude/share/man/man1/avrdude.1

@stefanrueger
Copy link
Collaborator Author

libavrdude-avrintel.h is not exactly public header file but still needed

As I understand it, technically it is a public header, but @ndim's suggestion, which I have been following in this PR, forces the API to only #include <libavrdude.h>.

@stefanrueger
Copy link
Collaborator Author

CMake change seems to be missing so that additional internal include file

Correct. I am hoping that @ndim would do the suggested change in his overhaul of the Make/build system.

So the plan is to merge this PR first and rest is done later in PR #1681

@mcuee
Copy link
Collaborator

mcuee commented Feb 14, 2024

CMake change seems to be missing so that additional internal include file

Correct. I am hoping that @ndim would do the suggested change in his overhaul of the Make/build system.

So the plan is to merge this PR first and rest is done later in PR #1681

I see. I am okay with this plan.

@Youw
Copy link
Contributor

Youw commented Feb 14, 2024

@mcuee yes it is valid and required as per current implementation.

@ndim
Copy link
Contributor

ndim commented Feb 14, 2024

I have taken a look at this branch and at libavrdude.h and libavrdude-avrintel.h and ended up with a few more changes at https://github.com/ndim/avrdude/tree/avrintel.h (rebased onto main with the 32bit msys2 builds removed so that CI succeeds again).

Any comments on that? Do you want to cherry-pick from that, or should I file a separate PR superseding this PR, or...?

@stefanrueger stefanrueger merged commit 64f2a3e into avrdudes:main Feb 14, 2024
11 of 13 checks passed
@ndim
Copy link
Contributor

ndim commented Feb 15, 2024

JFTR, I have put my changes into PR #1688.

@stefanrueger stefanrueger deleted the avrintel.h branch February 22, 2024 09:32
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.

4 participants