Fixes to build scripts for initial release #167

omus · 2023-09-28T21:20:49Z

In attempting to do our initial release I encountered a few issues with things being out of date.

As I worked through these issues I noticed that the artifacts we are uploading have very specific triplets. Normally, the triplets from BinaryBuilder are based upon BinaryBuilder.supported_platforms() which uses quite generic triplets and mainly focuses on OS and architecture. Having an overly specific triplet is an issue in that it can result in us generating more artifacts than we have to. I've reduced the triplet down to what should be the current minimum. Ideally, we wouldn't use the Julia version in our triplet but appears that our WORKSPACE.bazel setup is pulling in Julia specific headers which is resulting in shared libraries which are incompatible with other Julia version (I think it's because our library is fully statically compiled).

Anyway, these should be the final changes needed to be merged so we can cut our initial release.

omus · 2023-09-28T21:23:31Z

build/bind_artifacts.jl

-        if isnothing(m)
-            throw(ArgumentError("Could not parse host triplet from $(basename(artifact_path))"))
-        end
+    for platform in REQUIRED_PLATFORMS


By using a list of required platforms we can ensure that if we failed to upload all of the artifacts that the bind_artifacts.jl script will fail as it will attempt to download a file that doesn't exist

omus · 2023-09-28T21:24:48Z

build/bind_artifacts.jl

-        platform_str = "$platform_triplet-julia_version+$julia_version"
-        platform = parse(BinaryPlatforms.Platform, platform_str)
+        @info "Dowloading artifact $artifact_url"
+        artifact_path = joinpath(TARBALL_DIR, artifact_name)


Previously we downloaded files to a temporary directory instead of the TARBALL_DIR. This will override any locally generated files but that seems fine.

omus · 2023-09-28T21:25:54Z

build/common.jl

+const REQUIRED_JULIA_VERSIONS = (v"1.8", v"1.9")
+const REQUIRED_PLATFORMS = let
+    base_platforms = parse.(Platform, REQUIRED_BASE_TRIPLETS)
+    base_tags = [(; julia_version=string(v)) for v in REQUIRED_JULIA_VERSIONS]
+    [Platform(arch(p), platform_name(p); tags...)
+     for p in base_platforms, tags in base_tags][:]
+end


Didn't spend much time refactoring this. If we can make the artifacts non-Julia specific this becomes much cleaner

LGTM - it's not worth prettifying anyway

omus · 2023-09-28T21:27:07Z

build/common.jl

+function set_url_scheme(url, scheme)
+    m = match(LibGit2.URL_REGEX, url)
+    if m === nothing
+        throw(ArgumentError("URL is not a valid SCP or HTTP(S) URL: $(url)"))
+    end
+    return LibGit2.git_url(; scheme="https", username=something(m[:user], ""),
+                           host=something(m[:host], ""), port=something(m[:port], ""),
+                           path=something(m[:path], ""))
+end


@glennmoy I can understand your reason for copying the regex from LibGit2 as it's not exported by the library. However, now that I'm using git_url as well it seemed better to use the function and regex from the library that tests them.

So my understandig is this converts, e.g. git@github.com:beacon-biosignals/ray.jl to https://github.com:beacon-biosignals/ray.jl?

Yes, it converts SCP-like URLs to HTTPS URLs. Specifically:

julia> set_url_scheme("git@github.com:beacon-biosignals/Ray.jl", "https") "https://git@github.com/beacon-biosignals/Ray.jl"

We'll want to drop the user as we don't want that in our artifact URLs

build/common.jl

build/upload_tarballs.jl

omus · 2023-09-28T21:30:01Z

@glennmoy feel free to make changes, approve, and merge this when you get in. Once that's done we can make the v0.0.1 release.

glennmoy · 2023-09-29T09:36:12Z

As I worked through these issues I noticed that the artifacts we are uploading have very specific triplets. Normally, the triplets from BinaryBuilder are based upon BinaryBuilder.supported_platforms() which uses quite generic triplets and mainly focuses on OS and architecture. Having an overly specific triplet is an issue in that it can result in us generating more artifacts than we have to.

Yeah I wasn't sure about having to include libgfortran etc since we're doing this outside BinaryBuilder...I went looking for alternative functions but concluded that perhaps triplet() was the canonical way to generate the artifact names? Seems it can be overly specific but maybe that's by design so the user can decide what not to include?

glennmoy

I'll give this a run through to make sure it works before merging

build/bind_artifacts.jl

glennmoy · 2023-09-29T09:38:56Z

build/common.jl

+const REQUIRED_JULIA_VERSIONS = (v"1.8", v"1.9")
+const REQUIRED_PLATFORMS = let
+    base_platforms = parse.(Platform, REQUIRED_BASE_TRIPLETS)
+    base_tags = [(; julia_version=string(v)) for v in REQUIRED_JULIA_VERSIONS]
+    [Platform(arch(p), platform_name(p); tags...)
+     for p in base_platforms, tags in base_tags][:]
+end


LGTM - it's not worth prettifying anyway

glennmoy · 2023-09-29T09:42:06Z

build/common.jl

+function set_url_scheme(url, scheme)
+    m = match(LibGit2.URL_REGEX, url)
+    if m === nothing
+        throw(ArgumentError("URL is not a valid SCP or HTTP(S) URL: $(url)"))
+    end
+    return LibGit2.git_url(; scheme="https", username=something(m[:user], ""),
+                           host=something(m[:host], ""), port=something(m[:port], ""),
+                           path=something(m[:path], ""))
+end


So my understandig is this converts, e.g. git@github.com:beacon-biosignals/ray.jl to https://github.com:beacon-biosignals/ray.jl?

build/common.jl

glennmoy · 2023-09-29T09:48:15Z

build/upload_tarballs.jl

@@ -92,15 +64,9 @@ if abspath(PROGRAM_FILE) == @__FILE__
    # check that contents are tarballs with correct filename
    all(t -> !isnothing(match(TARBALL_REGEX, t)), readdir(TARBALL_DIR))


first time I wrote this I wanted to see if there was a file command (similar to the bash command) that would assert the file type rather than check against a regex. I went looking in Base.Filesystem module but didn't see anything. Only calling it out in case you knew of one off hand - not suggesting we have to change it now.

I think the usual way of handling this would be just to look at the extension. The other way is probably to use FileIO.jl and use the detection function which would detect the file type from magic bytes in the header: https://juliaio.github.io/FileIO.jl/stable/registry/#Registry-table

glennmoy · 2023-09-29T09:50:43Z

Assuming this works we'll merge it, I'll regenerate the Artifacts and then we can make a tag!

codecov · 2023-09-29T12:02:40Z

Codecov Report

Merging #167 (38f1530) into main (e2bc21e) will not change coverage.
The diff coverage is n/a.

❗ Current head 38f1530 differs from pull request most recent head edd6129. Consider uploading reports for the commit edd6129 to get more accurate results

@@           Coverage Diff           @@
##             main     #167   +/-   ##
=======================================
  Coverage   93.90%   93.90%           
=======================================
  Files          12       12           
  Lines         558      558           
=======================================
  Hits          524      524           
  Misses         34       34

Flag	Coverage Δ
Ray.jl	`93.90% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

glennmoy · 2023-09-29T12:55:27Z

@omus the build worked - I just have a few suggestions incoming

…Ray.jl into cv/build-script-fixes

build/upload_tarballs.jl

omus added 3 commits September 28, 2023 15:26

Fix artifact scripts

c8d5def

Fix .gitignore

9d642a2

Use more generic triplets

e8d2e75

omus requested a review from glennmoy September 28, 2023 21:20

omus self-assigned this Sep 28, 2023

Formatting

38f1530

omus commented Sep 28, 2023

View reviewed changes

omus mentioned this pull request Sep 28, 2023

The ray_julia library artifact is not Julia version agnostic #168

Open

glennmoy approved these changes Sep 29, 2023

View reviewed changes

glennmoy and others added 4 commits September 29, 2023 14:01

minor suggestions

3eeee8f

Always drop username from HTTPS URL

8fd2c1b

refactor slightly

6e37716

Merge branch 'cv/build-script-fixes' of github.com:beacon-biosignals/…

8b166fb

…Ray.jl into cv/build-script-fixes

omus commented Sep 29, 2023

View reviewed changes

build/upload_tarballs.jl Outdated Show resolved Hide resolved

even better

edd6129

omus commented Sep 29, 2023

View reviewed changes

build/upload_tarballs.jl Show resolved Hide resolved

omus merged commit e2262ab into main Sep 29, 2023
3 checks passed

omus deleted the cv/build-script-fixes branch September 29, 2023 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to build scripts for initial release #167

Fixes to build scripts for initial release #167

omus commented Sep 28, 2023

omus Sep 28, 2023

omus Sep 28, 2023

omus Sep 28, 2023

glennmoy Sep 29, 2023

omus Sep 28, 2023

glennmoy Sep 29, 2023

omus Sep 29, 2023 •

edited

Loading

omus commented Sep 28, 2023 •

edited

Loading

glennmoy commented Sep 29, 2023

glennmoy left a comment

glennmoy Sep 29, 2023

glennmoy Sep 29, 2023

glennmoy Sep 29, 2023

omus Sep 29, 2023

glennmoy commented Sep 29, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

glennmoy commented Sep 29, 2023

		@@ -92,15 +64,9 @@ if abspath(PROGRAM_FILE) == @__FILE__
		# check that contents are tarballs with correct filename
		all(t -> !isnothing(match(TARBALL_REGEX, t)), readdir(TARBALL_DIR))

Fixes to build scripts for initial release #167

Fixes to build scripts for initial release #167

Conversation

omus commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

omus commented Sep 28, 2023 • edited Loading

glennmoy commented Sep 29, 2023

glennmoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennmoy commented Sep 29, 2023

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

glennmoy commented Sep 29, 2023

omus Sep 29, 2023 •

edited

Loading

omus commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading