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

Makefile: Keep output artifacts when rebuilding #198

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

grische
Copy link
Contributor

@grische grische commented Oct 3, 2022

When doing a new build or a rebuild, all previous artifacts are deleted even if the new build fails. Replacing mv with rsync allows preserving previous artifacts.

When doing a new build or a rebuild, all previous artifacts are deleted even if
the new build fails. Replacing mv with rsync allows preserving old artifacts.
@grische grische marked this pull request as ready for review October 4, 2022 12:19
@grische grische requested a review from a team as a code owner October 4, 2022 12:19
Copy link
Member

@goligo goligo left a comment

Choose a reason for hiding this comment

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

Honestly, I don't like this - just complicates things, if old build artifacts are kept. You can no longer distinguish, whether they are from the current or last build, whether they include your latest change or they don't.

If you want to keep old build output, make a local script to copy them somewhere else.

@goligo goligo closed this Oct 4, 2022
@grische
Copy link
Contributor Author

grische commented Oct 4, 2022

Each build has both a timestamp as well as it uses the git hash in its name. I don't see how you be struggling to distinguish new and old builds, when builds take always more than a few minutes.

In case of identical names, successful builds will overwrite existing artifacts. If a build failed, you would be able to see it both in the log messages, the return code or the timestamp of the last build.

@grische grische reopened this Oct 4, 2022
Copy link
Member

@goligo goligo left a comment

Choose a reason for hiding this comment

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

Seriously, I think this is wrong. I don't want to keep old build output. If you really want this, hide it behind a build option and keep the default as is.

@grische grische added the next label Mar 5, 2023
@Djfe
Copy link

Djfe commented Aug 27, 2023

in Aachen we use environment variables now and only have one output folder for Gluon instead.
no mv or rsync needed. This way you have to move outputfolders if you want to keep old builds.

we set

GLUON_SITEDIR := ..  # can only be set in makefile, we don't need to create a symbolic link anylonger
GLUON_OUTPUTDIR := $(GLUON_SITEDIR)/output  # we are setting this in our site.mk

https://gluon.readthedocs.io/en/latest/user/getting_started.html

also we have a different output folder, for when we build single devices now:

ifdef GLUON_DEVICES
    GLUON_OUTPUTDIR := $(GLUON_SITEDIR)/devices
endif

(we stop output-clean, when GLUON_DEVICES is set)

@GoliathLabs
Copy link
Member

in Aachen we use environment variables now and only have one output folder for Gluon instead. no mv or rsync needed. This way you have to move outputfolders if you want to keep old builds.

we set

GLUON_SITEDIR := ..  # can only be set in makefile, we don't need to create a symbolic link anylonger
GLUON_OUTPUTDIR := $(GLUON_SITEDIR)/output  # we are setting this in our site.mk

https://gluon.readthedocs.io/en/latest/user/getting_started.html

also we have a different output folder, for when we build single devices now:

ifdef GLUON_DEVICES
    GLUON_OUTPUTDIR := $(GLUON_SITEDIR)/devices
endif

(we stop output-clean, when GLUON_DEVICES is set)

@grische what do you think?

@grische grische marked this pull request as draft November 6, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants