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

Show more information in get_git_commit #2468

Merged
merged 1 commit into from Nov 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/alpine-32bit-build-and-test.yaml
Expand Up @@ -30,7 +30,7 @@ jobs:

- name: Install build dependencies
run: |
apk add --no-cache --virtual .build-deps coreutils dpkg-dev findutils gcc libc-dev make util-linux-dev diffutils cmake openssl-dev sudo flex bison
apk add --no-cache --virtual .build-deps coreutils dpkg-dev findutils gcc libc-dev make util-linux-dev diffutils cmake openssl-dev sudo flex bison git
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this dependency now that Git is still optional? Isn't it good to also run some tests where we don't have Git installed and build the dummy version of the get_git_commit function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should require git also for this case. Before, the tests was designed to not fail when Git was not available, but that is not the case right now and it will fail if git is not available (because the function generate an error rather than some output).

Since this is an internal function intended for debugging, I do not think a full test is necessary, but if you feel strongly about testing this, we can create a separate test that is only added if Git is absent.


- name: Build pg_isolation_regress
run: |
Expand Down
2 changes: 1 addition & 1 deletion scripts/docker-build.sh
Expand Up @@ -54,7 +54,7 @@ create_postgres_build_image() {
docker run -d --name ${BUILD_CONTAINER_NAME} --env POSTGRES_HOST_AUTH_METHOD=trust -v ${BASE_DIR}:/src postgres:${PG_IMAGE_TAG}

# Install build dependencies
docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "apk add --no-cache --virtual .build-deps gdb git coreutils dpkg-dev gcc libc-dev make cmake util-linux-dev diffutils openssl-dev && mkdir -p /build/debug"
docker exec -u root ${BUILD_CONTAINER_NAME} /bin/bash -c "apk add --no-cache --virtual .build-deps gdb coreutils dpkg-dev gcc git libc-dev make cmake util-linux-dev diffutils openssl-dev && mkdir -p /build/debug"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like git is just moved to later in the list. Looks like this change isn't strictly needed in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I'll remove it if I make more changes to the PR.

docker commit -a $USER -m "TimescaleDB build base image version $PG_IMAGE_TAG" ${BUILD_CONTAINER_NAME} ${image}
remove_build_container ${BUILD_CONTAINER_NAME}

Expand Down
2 changes: 2 additions & 0 deletions sql/updates/latest-dev.sql
@@ -1,2 +1,4 @@
DROP VIEW IF EXISTS timescaledb_information.continuous_aggregates;
DROP VIEW IF EXISTS timescaledb_information.job_stats;
DROP FUNCTION IF EXISTS _timescaledb_internal.get_git_commit;

3 changes: 2 additions & 1 deletion sql/version.sql
Expand Up @@ -2,7 +2,8 @@
-- Please see the included NOTICE for copyright information and
-- LICENSE-APACHE for a copy of the license.

CREATE OR REPLACE FUNCTION _timescaledb_internal.get_git_commit() RETURNS TEXT
CREATE OR REPLACE FUNCTION _timescaledb_internal.get_git_commit()
RETURNS TABLE(commit_tag TEXT, commit_hash TEXT, commit_time TIMESTAMPTZ)
AS '@MODULE_PATHNAME@', 'ts_get_git_commit' LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

CREATE OR REPLACE FUNCTION _timescaledb_internal.get_os_info()
Expand Down
56 changes: 36 additions & 20 deletions src/CMakeLists.txt
Expand Up @@ -72,30 +72,46 @@ if (CMAKE_BUILD_TYPE MATCHES Debug)
endif (CMAKE_BUILD_TYPE MATCHES Debug)

include(build-defs.cmake)
set(GITCOMMIT_H ${CMAKE_CURRENT_BINARY_DIR}/gitcommit.h)

if (WIN32)
add_custom_command(
OUTPUT ${GITCOMMIT_H}
WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
COMMAND ${CMAKE_COMMAND} -E echo_append "#define EXT_GIT_COMMIT " > ${GITCOMMIT_H}
COMMAND (${GIT_EXECUTABLE} describe --abbrev=4 --dirty --always --tags 2> NUL || call && echo "${PROJECT_VERSION_MOD}") >> ${GITCOMMIT_H}
COMMENT "Generating gitcommit.h"
VERBATIM)
else ()
add_custom_command(
OUTPUT ${GITCOMMIT_H}
WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
COMMAND ${CMAKE_COMMAND} -E echo_append "#define EXT_GIT_COMMIT " > ${GITCOMMIT_H}
COMMAND sh -c "if `${GIT_EXECUTABLE} status > /dev/null 2>&1`; then ${GIT_EXECUTABLE} describe --abbrev=4 --dirty --always --tags ; else echo ${PROJECT_VERSION_MOD} ; fi" >> ${GITCOMMIT_H}
COMMENT "Generating gitcommit.h"
VERBATIM)
endif (WIN32)
if(GIT_FOUND)
erimatnor marked this conversation as resolved.
Show resolved Hide resolved
# We use "git describe" to generate the tag. It will find the latest
# tag and also add some additional information if we are not on the
# tag.
execute_process(
COMMAND ${GIT_EXECUTABLE} describe --dirty --always --tags
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
OUTPUT_VARIABLE EXT_GIT_COMMIT_TAG
RESULT_VARIABLE _describe_RESULT
OUTPUT_STRIP_TRAILING_WHITESPACE)

# Fetch the commit HASH of head (short version) using rev-parse
execute_process(
COMMAND ${GIT_EXECUTABLE} rev-parse --short HEAD
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
OUTPUT_VARIABLE EXT_GIT_COMMIT_HASH
RESULT_VARIABLE _revparse_RESULT
OUTPUT_STRIP_TRAILING_WHITESPACE)

# Fetch the date of the head commit
execute_process(
COMMAND ${GIT_EXECUTABLE} log -1 --format=%aI
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
OUTPUT_VARIABLE EXT_GIT_COMMIT_TIME
RESULT_VARIABLE _log_RESULT
OUTPUT_STRIP_TRAILING_WHITESPACE)

# Results are non-zero if there were an error
if(_describe_RESULT OR _revparse_RESULT OR _log_RESULT)
message(STATUS "Unable to get git commit information")
endif()
endif()

configure_file(gitcommit.h.in gitcommit.h)

if (CMAKE_BUILD_TYPE MATCHES Debug)
add_library(${PROJECT_NAME} MODULE ${SOURCES} ${GITCOMMIT_H} $<TARGET_OBJECTS:${TESTS_LIB_NAME}>)
add_library(${PROJECT_NAME} MODULE ${SOURCES} ${GITCOMMIT_H} $<TARGET_OBJECTS:${TESTS_LIB_NAME}>)
else ()
add_library(${PROJECT_NAME} MODULE ${SOURCES} ${GITCOMMIT_H})
add_library(${PROJECT_NAME} MODULE ${SOURCES} ${GITCOMMIT_H})
endif ()

if (SEND_TELEMETRY_DEFAULT)
Expand Down
10 changes: 10 additions & 0 deletions src/annotations.h
Expand Up @@ -17,4 +17,14 @@
#define TS_FALLTHROUGH /* FALLTHROUGH */
#endif

#ifdef __has_attribute
erimatnor marked this conversation as resolved.
Show resolved Hide resolved
#if __has_attribute(used)
#define TS_USED __attribute__((used))
#else
#define TS_USED
#endif
#else
#define TS_USED
#endif

#endif /* TIMESCALEDB_ANNOTATIONS_H */
14 changes: 14 additions & 0 deletions src/gitcommit.h.in
@@ -0,0 +1,14 @@
/*
* This file and its contents are licensed under the Apache License 2.0.
* Please see the included NOTICE for copyright information and
* LICENSE-APACHE for a copy of the license.
*/

#ifndef GITCOMMIT_H_
#define GITCOMMIT_H_

#cmakedefine EXT_GIT_COMMIT_TAG "@EXT_GIT_COMMIT_TAG@"
#cmakedefine EXT_GIT_COMMIT_HASH "@EXT_GIT_COMMIT_HASH@"
#cmakedefine EXT_GIT_COMMIT_TIME "@EXT_GIT_COMMIT_TIME@"

#endif /* GITCOMMIT_H_ */
72 changes: 64 additions & 8 deletions src/version.c
Expand Up @@ -10,32 +10,88 @@
#include <funcapi.h>
#include <fmgr.h>
#include <storage/fd.h>
#include <utils/timestamp.h>

#include "fmgr.h"
#include "compat.h"
#include "annotations.h"
#include "gitcommit.h"
#include "version.h"
#include "config.h"

#define STR_EXPAND(x) #x
#define STR(x) STR_EXPAND(x)
/* Export the strings to that we can read them using strings(1). We add a
* prefix so that we can easily find it using grep(1). We only bother about
* generating them if the relevant symbol is defined. */
#ifdef EXT_GIT_COMMIT_HASH
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work as expected?

Even if @EXT_GIT_COMMIT_TAG@ is empty in gitcommit.h.in, I suspect EXT_GIT_COMMIT_TAG will still be defined (albeit empty)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Still unclear what happens if EXT_GIT_COMMIT_HASH is defined like this:

#define EXT_GIT_COMMIT_HASH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is not defined like that in the generated file, it is either

#define EXT_GIT_COMMIT_HASH "2beeb3fc"

or

/* #undef EXT_GIT_COMMIT_HASH */

If a builder decides on editing the file and putting anything else there, they can break the build whatever we do, so there is no reason to handle that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I wanted to know.

static const char commit_hash[] TS_USED = "commit-hash:" EXT_GIT_COMMIT_HASH;
#endif

static const char *git_commit = STR(EXT_GIT_COMMIT);
#ifdef EXT_GIT_COMMIT_TAG
static const char commit_tag[] TS_USED = "commit-tag:" EXT_GIT_COMMIT_TAG;
#endif

#ifdef EXT_GIT_COMMIT_TIME
static const char commit_time[] TS_USED = "commit-time:" EXT_GIT_COMMIT_TIME;
#endif

TS_FUNCTION_INFO_V1(ts_get_git_commit);

/* Return git commit information defined in header file gitcommit.h. We
* support that some of the fields are defined and will only show the fields
* that are defined. If no fields are defined, we throw an error notifying the
* user that there is no git information available at all. */
#if defined(EXT_GIT_COMMIT_HASH) || defined(EXT_GIT_COMMIT_TAG) || defined(EXT_GIT_COMMIT_TIME)
Datum
ts_get_git_commit(PG_FUNCTION_ARGS)
{
size_t var_size = VARHDRSZ + strlen(git_commit);
text *version_text = (text *) palloc(var_size);
TupleDesc tupdesc;
HeapTuple tuple;
Datum values[3] = { 0 };
bool nulls[3] = { false };

SET_VARSIZE(version_text, var_size);
/* Build a tuple descriptor for our result type */
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("function returning record called in context "
"that cannot accept type record")));

memcpy((void *) VARDATA(version_text), (void *) git_commit, var_size - VARHDRSZ);
tupdesc = BlessTupleDesc(tupdesc);

PG_RETURN_TEXT_P(version_text);
#ifdef EXT_GIT_COMMIT_TAG
values[0] = CStringGetTextDatum(EXT_GIT_COMMIT_TAG);
#else
nulls[0] = true;
#endif

#ifdef EXT_GIT_COMMIT_HASH
values[1] = CStringGetTextDatum(EXT_GIT_COMMIT_HASH);
#else
nulls[1] = true;
#endif

#ifdef EXT_GIT_COMMIT_TIME
values[2] = DirectFunctionCall3(timestamptz_in,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know that the the Git timestamp is always a parsable TimestampTz string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AIUI, yes. The default is RFC2822, which is parseable by the timestamptz function: https://www.postgresql.org/message-id/32c009ea0510121301y6da7d43eq@mail.gmail.com

Documentation is vague, but says:

Date and time input is accepted in almost any reasonable format, including ISO 8601, SQL-compatible, traditional POSTGRES, and others.

CStringGetDatum(EXT_GIT_COMMIT_TIME),
Int32GetDatum(-1),
Int32GetDatum(-1));
#else
nulls[2] = true;
#endif

tuple = heap_form_tuple(tupdesc, values, nulls);

return HeapTupleGetDatum(tuple);
}
#else
Datum
ts_get_git_commit(PG_FUNCTION_ARGS)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("extension not built with any Git commit information")));
}
#endif

#ifdef WIN32

Expand Down
13 changes: 8 additions & 5 deletions test/expected/version.out
@@ -1,11 +1,14 @@
-- This file and its contents are licensed under the Apache License 2.0.
-- Please see the included NOTICE for copyright information and
-- LICENSE-APACHE for a copy of the license.
-- Test that get_git_commit returns text
select pg_typeof(git) from _timescaledb_internal.get_git_commit() AS git;
pg_typeof
-----------
text
-- Check what get_git_commit returns
SELECT pg_typeof(commit_tag) AS commit_tag_type,
pg_typeof(commit_hash) AS commit_hash_type,
pg_typeof(commit_time) AS commit_time_type
FROM _timescaledb_internal.get_git_commit();
commit_tag_type | commit_hash_type | commit_time_type
-----------------+------------------+--------------------------
text | text | timestamp with time zone
(1 row)

-- Test that get_os_info returns 3 x text
Expand Down
7 changes: 5 additions & 2 deletions test/sql/version.sql
Expand Up @@ -2,8 +2,11 @@
-- Please see the included NOTICE for copyright information and
-- LICENSE-APACHE for a copy of the license.

-- Test that get_git_commit returns text
select pg_typeof(git) from _timescaledb_internal.get_git_commit() AS git;
-- Check what get_git_commit returns
SELECT pg_typeof(commit_tag) AS commit_tag_type,
pg_typeof(commit_hash) AS commit_hash_type,
pg_typeof(commit_time) AS commit_time_type
FROM _timescaledb_internal.get_git_commit();

-- Test that get_os_info returns 3 x text
select pg_typeof(sysname) AS sysname_type,pg_typeof(version) AS version_type,pg_typeof(release) AS release_type from _timescaledb_internal.get_os_info();