search

rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
194 stars 55 forks source link

Support offline builds #1410

Closed dkg closed 3 years ago

dkg commented 3 years ago

Debian packages can't access the Internet when building. We do want to run the test suites though.

the googletest debian package offers a reasonable, maintained version of googletest source, and it can be used instead of downloading googletest from the Internet.

It would be great if it was straightforward to tell the build process to look for googletest source in /usr/src/googletest/googletest instead of fetching it from the Internet.

At the moment, I'm patching src/tests/CMakeLists.txt. I can keep on patching it if you don't want to make it configurable, but i'd prefer either autodetection, or to just provide a simple configuration variable, instead of having to maintain a patch.

dewyatt commented 3 years ago

We really should support building on systems without internet access, so we'll probably want to make this configurable I'd say.

dewyatt commented 3 years ago

Similar issue with version.cmake as well -- https://github.com/rnpgp/rnp/issues/1377

dkg commented 3 years ago

thanks! we're also bundling version.cmake with the debian build as you can see in that patch, so yeah fixing that would be useful for us too.

fwiw, given that the cmake instructions to fetch version.cmake are content-indexed (they contain a github URL with an embedded commit ID) it's not clear to me how they're better than just including version.cmake directly. It's not like there's a lot of code in that file, and it's not like you've got a dynamic URL that's going to automatically pull in updates if version.cmake changes upstream. So I'm not sure what the advantage is of having it stripped out as part of the build.

maybe the goal is to make it easier to update? in that case, maybe you just want a special build target that just pulls from the current head of upstream version.cmake, and commits any changes to the rnp repository directly. Then whoever is the release manager could run that target before making a release.

andreasstieger commented 3 years ago

version.cmake is already referenced by hash, and the hash is versioned: https://build.opensuse.org/request/show/872022 For the openSUSE package the solution is bundling this file.

dkg commented 3 years ago

@andreasstieger agreed -- when i said "content-indexed" i meant the same thing as your "referenced by hash". It'd be simpler if upstream just shipped the file directly.

(for debian, i might still patch it b/c i don't want any of the git auto-detection stuff to creep into the packaged binaries, but that's a separate issue)

dkg commented 3 years ago

I recommend re-opening this because #1428 only addressed the googletest issues, but did not address cmake/version.cmake.

it got moved out of this repo in 78d366c88c752b459bc36c33808b1490d2c34066 and is afaict now the only required external dependency needed to do the build.

ni4 commented 3 years ago

@dewyatt What do you think about returning version.cmake to the repository, and adding some CI step which checks whether it is up-to-date?

dewyatt commented 3 years ago

@dewyatt What do you think about returning version.cmake to the repository, and adding some CI step which checks whether it is up-to-date?

Not a problem IMO

ni4 commented 3 years ago

@dkg Is it good to close this now? All issues seems to be resolved.

dkg commented 3 years ago

yep, the master branch looks ok to me, though i haven't done extensive testing or experimentation. i'll re-open it (or open a new issue) if i run into any other trouble on a release. thanks for taking care of all this!