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

feat(ci): deb package on release #518

Merged
merged 9 commits into from
Nov 11, 2024
Merged

feat(ci): deb package on release #518

merged 9 commits into from
Nov 11, 2024

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Oct 16, 2024

Ref: #289

So I did an update to the toml file, the ci will use cargo deb to generate the package, tested locally and works.

I am not sure how the CI works, I think that I am putting the file in the right path but I am not sure if it is the right job.

@Mte90 Mte90 requested review from Ph0enixKM and b1ek October 16, 2024 10:36
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

I've just generated a new release on my testing forked repository. The action failed to add the debian installer to the Assets under the release. Here is what assets it generated:

image

@Mte90
Copy link
Member Author

Mte90 commented Oct 22, 2024

As I can see from the log https://github.com/Ph0enixKM/amber-workflow-tests/actions/runs/11464290559/job/31900143129 the deb package was generated.

I think that the issue is that I have to specify the file to upload because doesn't upload all the files in a specific folder.

@Mte90
Copy link
Member Author

Mte90 commented Oct 22, 2024

@Ph0enixKM can you try now?

@Ph0enixKM
Copy link
Member

@Mte90 I'll finish fixing master and then I'll try it again

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Oct 23, 2024

Let's merge this fix (#535) and then I'll test this branch.

@Mte90 Mte90 requested a review from Ph0enixKM October 24, 2024 08:40
@Ph0enixKM
Copy link
Member

After testing the new changes, it seems that it still does not work. I got no .deb in the assets section of a release.

  1. I just read this change and you have written this in the plan phase of the build setup. It would make more sense to move it into the building phase called build-global-artifacts.
  2. What arch do you want to compile the deb for? I see that the default runner architecture is x86_64-unknown-linux-gnu so it will probably compile to that. Please add compilation triple to the filename. You get get the target triple like so TARGET_TRIPLE="$(rustc -Vv | awk '/^host/ { print $2 }')". Should we support ARM?
  3. I think that you don't upload the generated .deb to the assets artifacts anywhere in the pipeline.

Here are the changes that should get this to work:
IMG_0546

@Mte90
Copy link
Member Author

Mte90 commented Oct 25, 2024

I did that changes, automatically the file is created with the architecture where it is compiled.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@Mte90 Mte90 requested a review from Ph0enixKM October 28, 2024 09:24
@@ -174,7 +174,10 @@ jobs:
pattern: artifacts-*
path: target/distrib/
merge-multiple: true

- name: Cargo Deb
Copy link
Member

Choose a reason for hiding this comment

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

i don't get it. i thought we still haven't discussed if it is our responsibility to maintain packages

i am making a code suggestion here so that this will be a discussion so it can be minimized when resolved

ref #294

Copy link
Member

Choose a reason for hiding this comment

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

can this at least be in a different repo in the organization? i'm pretty sure there is a million ways to get the tool to pull code from it

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually projects release the package for the various distro, in a different repo I think that is the case when the project will grrow.
Right now I can be the debian maintainer, as the package it is just the binarry generated from rust in a dedicated compressed package, is not something like for Nix.

Copy link
Member

@b1ek b1ek Oct 31, 2024

Choose a reason for hiding this comment

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

in a different repo I think that is the case when the project will grrow

normally i would agree to move it later, but files urls in releases are something people will use in scripts, so its not very good to modify or remove them. so it has to be moved there right away.

why even have it in the compiler repo? i don't think there is a valid reason for having it here rather than in a separate one

Copy link
Member

Choose a reason for hiding this comment

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

Debian packages are easy to use and not that hard to maintain. I see the reason to support just the most popular distributions so that we can maintain a good experience of the installation process.

@Ph0enixKM
Copy link
Member

@Mte90 it seems that the CI still fails

Define specific path to amber debian package so it's easier to debug.
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Tested and worked

@hdwalters hdwalters self-requested a review November 11, 2024 12:07
@Mte90 Mte90 merged commit 825f280 into amber-lang:master Nov 11, 2024
1 check passed
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.

4 participants