Skip to content

Add build tags to BUILDING.md#3687

Merged
milosgajdos merged 1 commit intodistribution:mainfrom
Jamstah:add-build-tags
Aug 20, 2022
Merged

Add build tags to BUILDING.md#3687
milosgajdos merged 1 commit intodistribution:mainfrom
Jamstah:add-build-tags

Conversation

@Jamstah
Copy link
Collaborator

@Jamstah Jamstah commented Jul 12, 2022

Easier to see what build tags there are and what they are used for.

Signed-off-by: James Hewitt [email protected]

Easier to see what build tags there are and what they are used for.

Signed-off-by: James Hewitt <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@milosgajdos PTAL

<dt>noresumabledigest</dt>
<dd>Compiles without resumable digest support</dd>
<dt>include_gcs</dt>
<dd>Adds support for <a href="https://cloud.google.com/storage">Google Cloud Storage</a></dd>
Copy link
Member

Choose a reason for hiding this comment

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

I've noticed in some issues conversation that this seems to be broken -- just wondering if it's worth keeping it here even if that's the case, until we've fixed it

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that was the same PR that "triggered" this PR; #3676 (comment)

I think it's ok to keep it here, because it's meant to be working, but we should have a stage in CI that also at least tries to build the binaries with these build-tags enabled.

Possibly we also need to add these build-tags to GolangCI lint (ISTR it skips linting otherwise)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its actually already in the CI:

DOCKER_BUILDTAGS: "include_oss include_gcs"

However, the CI runs on go 1.17. It compiles fine on go 1.17.

Copy link
Member

Choose a reason for hiding this comment

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

Hm.. but CI also runs on go 1.18? Or is that only for "some" of the steps? #3643

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I just saw that, looking into it

Copy link
Collaborator Author

@Jamstah Jamstah Jul 12, 2022

Choose a reason for hiding this comment

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

OK, so I was wrong. That env var isn't doing anything because the Makefile actually wants BUILDTAGS. We should remove that env var from the yaml I guess.

Also, I used the wrong tag when checking on a go 1.17 build container. It doesn't actually compile on 1.17. So ignore me, the code doesn't work at the moment at all.

I still think its fine to merge this, and think that part of #3676 should be to add include_gcs to the CI. We should add include_oss as well somewhere.

@milosgajdos milosgajdos merged commit bc6b745 into distribution:main Aug 20, 2022
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