-
Notifications
You must be signed in to change notification settings - Fork 57
v2 - rename rawtoaces_idt to rawtoaces_core #160
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
v2 - rename rawtoaces_idt to rawtoaces_core #160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that a breaking change? I don't fully understand how it all works, but it is possible someone has rawtoaces/rta.h import in their project?
Do we want make it breaking (because we will bump the version?) Or we want capability to stay for now?
In which case I understand this file has to stay as
// Compatibility header for legacy includes. Will be removed in a future release.
#pragma once
#if defined(_MSC_VER)
#pragma message("rawtoaces: <rawtoaces/rta.h> is deprecated; use <rawtoaces/rawtoaces_core.h>")
#else
#warning "rawtoaces: <rawtoaces/rta.h> is deprecated; use <rawtoaces/rawtoaces_core.h>"
#endif
#include <rawtoaces/rawtoaces_core.h>| #ifndef _RTA_h__ | ||
| #define _RTA_h__ | ||
|
|
||
| #include "define.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple lines above should
#ifndef _RTA_h__
#define _RTA_h__be replaced with smth like
#ifndef _RAWTOACES_CORE_H__
#define _RAWTOACES_CORE_H__
include/rawtoaces/rawtoaces_core.h
Outdated
|
|
||
| #include <stdint.h> | ||
| #include <libraw/libraw.h> | ||
| #include <libraw.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not both?
#if defined(__has_include)
# if __has_include(<libraw/libraw.h>)
# include <libraw/libraw.h>
# elif __has_include(<libraw.h>)
# include <libraw.h>
# else
# error "LibRaw headers not found. Ensure libraw is installed and include paths are set."
# endif
#else
# include <libraw/libraw.h>
#endifyou'd need a change in src/rawtoaces_core/CMakeLists.txt to go along the way.
LibRaw is included in two common ways across systems:
#include <libraw/libraw.h> expects the compiler to search the parent include dir (e.g., /usr/local/include).
#include <libraw.h> expects the compiler to search the subdir itself (e.g., /usr/local/include/libraw).
Without adding both paths here, one of the include styles would fail in environments where only the other path is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot we are going to remove libraw in the next PR anyway. so, prob. ignore then
src/rawtoaces_core/CMakeLists.txt
Outdated
| if ( LIBRAW_CONFIG_FOUND ) | ||
| target_link_libraries ( ${RAWTOACES_CORE_LIB} PUBLIC libraw::raw ) | ||
| else () | ||
| target_include_directories(${RAWTOACES_CORE_LIB} PUBLIC ${libraw_INCLUDE_DIR} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one change I've mentioned in prev. comment is on this line:
get_filename_component(_libraw_parent_inc ${libraw_INCLUDE_DIR} DIRECTORY)
target_include_directories(${RAWTOACES_CORE_LIB} PUBLIC ${_libraw_parent_inc} ${libraw_INCLUDE_DIR} )I understand it would include prent of where libraw lives as well to search paths list
| LIBRARY DESTINATION ${INSTALL_LIB_DIR} | ||
| INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} | ||
| PUBLIC_HEADER DESTINATION include/rawtoaces | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add
add_library(rawtoaces_idt INTERFACE)
target_link_libraries(rawtoaces_idt INTERFACE ${RAWTOACES_CORE_LIB})
install( TARGETS rawtoaces_idt EXPORT RAWTOACESTargets )for backward compatibility so that those who previously linked rawtoaces_idt would not blow up?
Unless we want to make it a breaking change?
|
I don't see a changelog in this project, so maybe at least in README we can add something like Note: The legacy header `rawtoaces/rta.h` and target name `rawtoaces_idt` are deprecated.
Use `rawtoaces/rawtoaces_core.h` and the `rawtoaces_core` target.
A compatibility header and exported interface target `rawtoaces_idt` are provided for a transitional period.in case we keep things backward compatible. Or other text if we intend to break it. |
0702922 to
583ef48
Compare
the changelog is in docs/CHANGES.md. I have created it for v1.1. It doesn't contain any of the v2.0 notes yet, but we will definitely list all breaking changes there. |
Signed-off-by: Anton Dukhovnikov <[email protected]>
583ef48 to
ed1e3eb
Compare
a0db063
into
AcademySoftwareFoundation:master
No description provided.