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

Feature/simplified build rebased #2

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

florianesser-tng
Copy link

Provide github build action for reliable and reproducable builds

@florianesser-tng florianesser-tng force-pushed the feature/simplified-build-rebased branch 4 times, most recently from 9963737 to 6e4e080 Compare November 21, 2024 13:01
so I can develop on my branch but already test the workflow

Signed-off-by: Florian Esser <[email protected]>
 - fix bug, where python lib basicsr requires pytorch already
   being present at pip install time => pip  install must be
   executed in two steps, first install pytorch, then every-
   thing else
 - fix file encoding of license.rft: the encoding was some dos specific
   crosscompiling the electron app caused problems; the license could
   also not be rendered completely; it contained weird symbols when
   the installer was compiled from dos system
   unix fix: iconv -f windows-1252 -t utf-8 license.rft
 - structure build scripts a little bit, so they are easier to understand
 - use cross-env in npm commands, so paths specified in package.json work
   in all envs.

Signed-off-by: Florian Esser <[email protected]>
most of the resources required to build the project
you can easily get by a simply wget command.
The future of the conda dependency is unclear to me
hence require manual effort there instead of guessing
an incertain solution.

Signed-off-by: Florian Esser <[email protected]>
exclude conda dependency from build
this is OS dependent

Signed-off-by: Florian Esser <[email protected]>
Signed-off-by: Florian Esser <[email protected]>
let OS specifics be handled by action
instead of hassling with it.

Signed-off-by: Florian Esser <[email protected]>
Signed-off-by: Florian Esser <[email protected]>
instead of a hardcoded ref,
find the installer.exe by file name
ending.

Signed-off-by: Florian Esser <[email protected]>
by github action

Signed-off-by: Florian Esser <[email protected]>
to prevent confusion with and within github actions

Signed-off-by: Florian Esser <[email protected]>
- name: build installer
working-directory: "WebUI"
run: |
npm install

Choose a reason for hiding this comment

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

Is this second npm install necessary?

Copy link
Author

Choose a reason for hiding this comment

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

no, thanks for spotting

- symlink or copy the conda env path into /python_package_resources
3. run ```npm run pack-python```

The resulting `WebUI/npm_package_res/env.7z` could be reused for all platforms.

Choose a reason for hiding this comment

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

In what circumstance does this need to be executed and reference to GitHub action

const platformSpecificRequirementsTxt = existingFileOrExit(path.join(__dirname, '..', '..', 'service', `requirements-${platform}.txt`));
const requirementsTxt = existingFileOrExit(path.join(__dirname, '..', '..', 'service', `requirements-${platform}.txt`));
runPipInstall(platformSpecificRequirementsTxt, offlineEnvDir)
runPipInstall(requirementsTxt, offlineEnvDir)

Choose a reason for hiding this comment

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

Two identical tasks? Seperations between offline/online

process.exit(1);
}
});
}

Choose a reason for hiding this comment

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

Pip install seems to be missing

Copy link

@marijnvg-tng marijnvg-tng left a comment

Choose a reason for hiding this comment

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

see the comments...

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.

2 participants